-
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
Add stdlib version check for C++20 #3754
Conversation
include/fmt/core.h
Outdated
#if (FMT_CPLUSPLUS >= 202002L || \ | ||
(FMT_CPLUSPLUS >= 201709L && FMT_GCC_VERSION >= 1002)) && \ | ||
((!defined(_GLIBCXX_RELEASE) || _GLIBCXX_RELEASE >= 10) && \ | ||
(!defined(_LIBCPP_VERSION) || _LIBCPP_VERSION >= 10000) && \ | ||
(!FMT_MSC_VERSION || FMT_MSC_VERSION >= 1928)) |
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 the idea behind this change? Shall we enable FMT_CONSTEXPR20
only if is_constant_evaluated
is available instead? Something like
#if defined(FMT_HAS_IS_CONSTANT_EVALUATED) && \
((FMT_CPLUSPLUS >= 202002L) && \
(!defined(_GLIBCXX_RELEASE) || _GLIBCXX_RELEASE > 9)) || \
(FMT_CPLUSPLUS >= 201709L && FMT_GCC_VERSION >= 1002)
# define FMT_CONSTEXPR20 constexpr
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.
Main idea: Enable C++20 constexpr only if stdlib support C++20.
The condition (!defined(_GLIBCXX_RELEASE) || _GLIBCXX_RELEASE > 9)
partially check stdlib C++20 features. I extended it for libcxx and MSVC.
FMT_CONSTEXPR20
enabled if compiler support C++20 AND the version of stdlib is modern to support C++20.
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 compiler does not define _LIBCPP_VERSION in Android's case.
This will result in FMT_CONSTEXPR20 being incorrectly set to constexpr.
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.
Try to find the __config
file in the source code of your standard library and look at the value of the _LIBCPP_VERSION
macro.
Or in repository, like this:
https://android.googlesource.com/platform/external/libcxx/+/refs/tags/android-cts-10.0_r15/include/__config#36
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 see # define _LIBCPP_VERSION 170000
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
Interesting!
libcxx version 170000 fully supports C++20 and you shouldn't have issue #3745.
Can you compile and run the program:
#include <iostream>
int main()
{
std::cerr << _LIBCPP_VERSION << std::endl;
return 0;
}
in issue #3745 build 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.
I think we should just go with enabling C++20 constexpr only if consteval
is also available.
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.
Done.
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.
Sorry, I meant is_constant_evaluated
because that's what causing the OP's problem.
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.
@vitaut
Done.
Signed-off-by: Vladislav Shchapov <vladislav@shchapov.ru>
Thanks! |
Sorry I did not have the time to validate the fix 😞 It looks like it should be working. |
Signed-off-by: Vladislav Shchapov <vladislav@shchapov.ru>
Possible fix #3745