Conversation
vitaut
left a comment
vitaut
left a comment
There was a problem hiding this comment.
Thanks for the PR! The change LGTM but please move the test to base-test and check the actual API being changed, namely basic_string_view's ctor.
Makes sense, done. |
| #if FMT_HAS_BUILTIN(__builtin_strlen) || FMT_GCC_VERSION || FMT_CLANG_VERSION | ||
| if (std::is_same<Char, char>::value) { | ||
| if (std::is_same<Char, char>::value && !detail::is_constant_evaluated()) { | ||
| size_ = __builtin_strlen(detail::narrow(s)); |
There was a problem hiding this comment.
What's the advantage of still using __builtin_strlen? Will gcc or clang still end up producing a constant in some scenarios that they wouldn't if just strlen was used?
There was a problem hiding this comment.
Well... libstdc++ __builtin_strlen in char_traits, so I guess there's that.
There was a problem hiding this comment.
There was some benefit to the debug codegen at some point but probably no longer matters on newer compilers.
There was a problem hiding this comment.
More importantly, strlen is not constexpr.
|
Merged, thanks! |
3 participants
The added test fails on gcc 14 because of the call to
__builtin_strlen, so just avoiding it during constant evaluation. Fixed #4423.