-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Enable constexpr support for fmt::format (fmtlib#3403) #4456
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
base: master
Are you sure you want to change the base?
Enable constexpr support for fmt::format (fmtlib#3403) #4456
Conversation
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.
Thanks for the PR!
include/fmt/base.h
Outdated
@@ -141,6 +141,33 @@ | |||
# define FMT_CONSTEXPR20 | |||
#endif | |||
|
|||
// Detect constexpr std::string. | |||
#if !FMT_USE_CONSTEVAL |
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.
Why does it depend on consteval?
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.
FMT_USE_CONSTEVAL
enables FMT_CONSTEXPR20
which is required.
I could add comment or introduce something FMT_USE_CONSTEXPR20
test/compile-test.cc
Outdated
constexpr auto result = []() { | ||
return fmt::format(FMT_COMPILE("{}"), true) == "true"; | ||
}(); |
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.
Do we need a lambda here?
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.
Yes, unfortunately we do need it. The issue is that all allocated memory at compile time should be deallocated.
That means we are not able to write like this:
constexpr auto s = fmt::format(FMT_COMPILE("{}"), true)); // leaks allocated memory at compile-time to runtime.
EXPECT_EQ("true", s);
Alternatively we could use static_assert:
static_assert(fmt::format(FMT_COMPILE("{}"), true) == "true");
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.
Let's keep the lambda for now but I don't think we need to test different arguments since this is already tested elsewhere. What we need to test here is that fmt::format
can be called at compile time so I suggest picking on type of an argument (e.g. int
) and testing it.
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.
I have replaced boolean test cases with int
and drop test case for a type with user provided formatter.
0363675
to
8468cc5
Compare
# define FMT_USE_CONSTEXPR_STRING 0 | ||
#elif !defined(__cpp_lib_constexpr_string) | ||
# define FMT_USE_CONSTEXPR_STRING 0 | ||
#elif defined(__cpp_lib_constexpr_string) && \ |
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.
defined(__cpp_lib_constexpr_string)
seems redundant since we only enter this branch if !defined(__cpp_lib_constexpr_string)
is false.
8468cc5
to
6a0595d
Compare
6a0595d
to
7e7a4e9
Compare
This PR allows to use fmt::format at compile-time by: