-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Update isfinite declaration to be constexpr for c++23 only #3746
Conversation
f009126
to
04338ce
Compare
Question about the environment: |
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.
@phprus made a good point and it looks like a bug in is_constant_evaluated
and not isfinite
.
Granted, I might have been too quick in diagnosing the issue |
The implementation failed to be constexpr in the situation where: __cplusplus == 202002L && !defined(__cpp_lib_is_constant_evaluated) Fix fmtlib#3745
04338ce
to
f68bff6
Compare
@@ -2752,7 +2752,7 @@ template <typename T, FMT_ENABLE_IF(std::is_floating_point<T>::value&& | |||
has_isfinite<T>::value)> | |||
FMT_CONSTEXPR20 bool isfinite(T value) { | |||
constexpr T inf = T(std::numeric_limits<double>::infinity()); | |||
if (is_constant_evaluated()) | |||
if (is_constant_evaluated(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.
One drawback is that it switches to a slightly worse implementation on older systems: https://www.godbolt.org/z/WWzjEsrxj. But I guess we can optimize it later.
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 don't see an alternative other than testing FMT_CONSTEXPR20
in the body to replace is_constant_evaluated
:(
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 can use __builtin_is_constant_evaluated
instead of std::is_constant_evaluated
.
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.
Or we can improve
Lines 108 to 110 in 89860eb
#if ((FMT_CPLUSPLUS >= 202002L) && \ | |
(!defined(_GLIBCXX_RELEASE) || _GLIBCXX_RELEASE > 9)) || \ | |
(FMT_CPLUSPLUS >= 201709L && FMT_GCC_VERSION >= 1002) |
condition by adding a stdlib version check.
@hchataing, which version of libc++ (_LIBCPP_VERSION
) or libstdc++ (_GLIBCXX_RELEASE
) you are using?
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.
@hchataing check PR #3754 please
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.
Neither macro is defined by the compiler... but maybe I am checking wrong (clang++ -std=c++20 -E -dM foo.cc
)
Closing in favor of #3754 but thanks for the effort. |
Fix #3745