-
Notifications
You must be signed in to change notification settings - Fork 2.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix case of variant which is valueless by exception #3347
Conversation
Thanks for the PR. Please add a test case to https://github.com/fmtlib/fmt/blob/master/test/std-test.cc. |
@vitaut I have added the test case, and verified that it is working as intended. |
test/std-test.cc
Outdated
@@ -95,6 +96,28 @@ TEST(std_test, optional) { | |||
#endif | |||
} | |||
|
|||
// this struct exists only to force a variant to be | |||
// valueless by exception | |||
// NOLINTNEXTLINE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The purpose of the NOLINTNEXTLINE
is to suppress clang-tidy warnings concerning the throws_on_move
struct which is guaranteed to throw on move construction. I have removed it.
test/std-test.cc
Outdated
template <> struct fmt::formatter<throws_on_move_t> : formatter<string_view> { | ||
template <typename FormatContext> | ||
auto format(const throws_on_move_t&, FormatContext& ctx) const { | ||
string_view str{"<throws_on_move_t>"}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't use brace initialization except for initializer lists, let's replace with normal initialization.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My apologies, fixed.
test/std-test.cc
Outdated
template <typename FormatContext> | ||
auto format(const throws_on_move_t&, FormatContext& ctx) const { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't need to be a template, you can use format_context
instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
test/std-test.cc
Outdated
@@ -126,6 +149,20 @@ TEST(std_test, variant) { | |||
|
|||
volatile int i = 42; // Test compile error before GCC 11 described in #3068. | |||
EXPECT_EQ(fmt::format("{}", i), "42"); | |||
|
|||
using V2 = std::variant<std::monostate, throws_on_move_t>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This alias is not needed, just use the variant type below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
test/std-test.cc
Outdated
|
||
using V2 = std::variant<std::monostate, throws_on_move_t>; | ||
|
||
V2 v6{}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
{}
is redundant, please remove.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
test/std-test.cc
Outdated
// this struct exists only to force a variant to be | ||
// valueless by exception | ||
// NOLINTNEXTLINE | ||
struct throws_on_move_t { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_t
suffix is normally used for aliases (typedefs), please remove.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
test/std-test.cc
Outdated
V2 v6{}; | ||
|
||
try { | ||
throws_on_move_t thrower{}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
test/std-test.cc
Outdated
}; | ||
|
||
template <> struct fmt::formatter<throws_on_move> : formatter<string_view> { | ||
auto format(const throws_on_move&, format_context& ctx) const { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs a return type for compatibility with older compilers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My apologies once again. I've fixed this.
Almost there. Just one more issue on older compilers: https://www.internalfb.com/code/fbsource/[ae2a1f4ad0986b7c6686fa486466a9f6f47e9c05]/fbcode/thrift/compiler/test/fixtures/constants/gen-cpp2/module_constants.h?lines=140. |
I am unable to view this link. |
That ought to fix it. |
Thank you! |
This tiny repository contains a small bit of code which forces an
std::variant
to be valueless by exception, and demonstrates that upstream fmtlib does not handle that case.My small change simply handles this case.