Skip to content
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

Made basic_string_view's const Char* constructor constexpr in C++17. #2525

Closed
wants to merge 1 commit into from

Conversation

mwinterb
Copy link
Contributor

Also added commentary for why std::strlen is directly called in certain situations.
Addresses #2455 (comment).

Also added commentary for why std::strlen is directly called in certain situations.
Addresses fmtlib#2455 (comment).
basic_string_view(const Char* s) : data_(s), size_(0) {
// This test is needed to ensure that this constructor is constexpr in C++17
// modes.
#ifdef __cpp_lib_is_constant_evaluated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

__cpp_lib_is_constant_evaluated is defined only in C++20. Therefore, code generation optimizations will not work until C++20.

@vitaut
Copy link
Contributor

vitaut commented Oct 2, 2021

Thanks for the PR but I'd rather not introduce more conditional compilation there. Here's an alternative fix (still need to confirm once we get a repro): 023c201.

@vitaut vitaut closed this Oct 2, 2021
@mwinterb mwinterb deleted the const_eval_char_traits branch October 2, 2021 17:23
@mwinterb
Copy link
Contributor Author

mwinterb commented Oct 2, 2021

One thing to note is that constexpr char_traits comes with C++17, and both GCC and MSVC demand initialization of size_ in the member initializer list in their C++17 modes: https://gcc.godbolt.org/z/s8874qYWe. Since the original report is calling that constructor by a consteval function, obviously C++20 constexpr rules apply, so it may not matter at all for that specific repro. If it helps figure out if that constructor still needs to be constexpr in C++17, 664dd88 is when that constructor was first made constexpr.

https://gcc.godbolt.org/z/rPbTP433f for a further reduced version to illustrate that both want the initialization in the initializer list, the fact that the initialization is "conditional" in basic_string_view isn't the problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants