fix #2210 #2211
fix #2210 #2211
Conversation
include/fmt/core.h
Outdated
| FMT_INLINE basic_string_view(const Char* s) : data_(s) { | ||
| if (std::is_same<Char, char>::value && !detail::is_constant_evaluated()) | ||
| FMT_CONSTEXPR_CHAR_TRAITS_LENGTH FMT_INLINE basic_string_view(const Char* s) : data_(s) { | ||
| if FMT_CONSTEXPR_CHAR_TRAITS_LENGTH (std::is_same<Char, char>::value && !detail::is_constant_evaluated()) |
There was a problem hiding this comment.
This line becomes wrong with FMT_CONSTEXPR_CHAR_TRAITS_LENGTH replaced as FMT_CONSTEXPR, i.e. constexpr. From cppreference about std::is_constant_evaluated() (which is used inside detail::is_constant_evaluated() when supported):
When directly used as the condition of static_assert declaration or constexpr if statement, std::is_constant_evaluated() always returns true.
So it shouldn't be if constexpr, just if.
There was a problem hiding this comment.
The correct way to fix this warning I can imagine (probably it's also very unclear): combine __cplusplus check together with __cpp_lib_is_constant_evaluated check to use if constexpr only if detail::is_constant_evaluated() unconditionally false.
There was a problem hiding this comment.
Just replace FMT_CONSTEXPR_CHAR_TRAITS_LENGTH with detail::const_check and revert other changes.
include/fmt/core.h
Outdated
| // C++17's char_traits::length() is constexpr. | ||
| #if __cplusplus >= 201703L | ||
| # define FMT_CONSTEXPR_CHAR_TRAITS_LENGTH FMT_CONSTEXPR | ||
| #else | ||
| # define FMT_CONSTEXPR_CHAR_TRAITS_LENGTH | ||
| #endif | ||
|
|
There was a problem hiding this comment.
Also I'm not sure if it's worth introducing this special definition to use it only once.
| @@ -392,11 +399,8 @@ template <typename Char> class basic_string_view { | |||
| the size with ``std::char_traits<Char>::length``. | |||
| \endrst | |||
| */ | |||
| #if __cplusplus >= 201703L // C++17's char_traits::length() is constexpr. | |||
| FMT_CONSTEXPR | |||
There was a problem hiding this comment.
And another observation, it's not about this PR, just these lines in general: probably constexpr can be used here, not FMT_CONSTEXPR, since it's already under the __cplusplus >= 201703L macro check.
There was a problem hiding this comment.
Looks like this actually can be applied to this PR too, since it's also used in newly introduced macro definition.
|
Anyway, feel free to ignore my review, and just wait for comments from maintainers 🙂 |
include/fmt/core.h
Outdated
| FMT_INLINE basic_string_view(const Char* s) : data_(s) { | ||
| if (std::is_same<Char, char>::value && !detail::is_constant_evaluated()) | ||
| FMT_CONSTEXPR_CHAR_TRAITS_LENGTH FMT_INLINE basic_string_view(const Char* s) : data_(s) { | ||
| if FMT_CONSTEXPR_CHAR_TRAITS_LENGTH (std::is_same<Char, char>::value && !detail::is_constant_evaluated()) |
There was a problem hiding this comment.
Just replace FMT_CONSTEXPR_CHAR_TRAITS_LENGTH with detail::const_check and revert other changes.
include/fmt/core.h
Outdated
| @@ -392,11 +392,12 @@ template <typename Char> class basic_string_view { | |||
| the size with ``std::char_traits<Char>::length``. | |||
| \endrst | |||
| */ | |||
| #if __cplusplus >= 201703L // C++17's char_traits::length() is constexpr. | |||
| FMT_CONSTEXPR | |||
| // C++17's char_traits::length() is constexpr. | |||
There was a problem hiding this comment.
Looks good but please move this comment back to the next line to avoid interfering with an apidoc comment above and apply clang-format.
include/fmt/core.h
Outdated
| @@ -392,11 +392,14 @@ template <typename Char> class basic_string_view { | |||
| the size with ``std::char_traits<Char>::length``. | |||
| \endrst | |||
| */ | |||
| #if __cplusplus >= 201703L // C++17's char_traits::length() is constexpr. | |||
| FMT_CONSTEXPR | |||
| #if __cplusplus >= 201703L | |||
There was a problem hiding this comment.
Please revert back // C++17's char_traits::length() is constexpr. comment to this line.
include/fmt/core.h
Outdated
| @@ -392,11 +392,14 @@ template <typename Char> class basic_string_view { | |||
| the size with ``std::char_traits<Char>::length``. | |||
| \endrst | |||
| */ | |||
| #if __cplusplus >= 201703L // C++17's char_traits::length() is constexpr. | |||
| FMT_CONSTEXPR | |||
| #if __cplusplus >= 201703L // C++17's char_traits::length() is constexpr. | |||
There was a problem hiding this comment.
Apply clang-format here, please
| FMT_INLINE | ||
| basic_string_view(const Char* s) | ||
| : data_(s) { |
There was a problem hiding this comment.
Yeah, it's:
#if __cplusplus >= 201703L // C++17's char_traits::length() is constexpr.
FMT_CONSTEXPR
#endif
FMT_INLINE
basic_string_view(const Char* s) : data_(s) {or:
#if __cplusplus >= 201703L // C++17's char_traits::length() is constexpr.
FMT_CONSTEXPR
#endif
FMT_INLINE basic_string_view(const Char* s) : data_(s) {or:
#if __cplusplus >= 201703L // C++17's char_traits::length() is constexpr.
constexpr
#endif
FMT_INLINE
basic_string_view(const Char* s)
: data_(s) {
|
Merged, thanks. |
No description provided.