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

Implement P2291R3 constexpr Integral <charconv> #3049

Merged
merged 9 commits into from
Aug 27, 2022

Conversation

AdamBucior
Copy link
Contributor

@AdamBucior AdamBucior commented Aug 23, 2022

Fixes #2920.

@AdamBucior AdamBucior requested a review from a team as a code owner August 23, 2022 17:30
@cpplearner
Copy link
Contributor

I tried to just extend that test but it didn't work well due to constexpr steps limit

😈 Should we increase the limit?

@AdamBucior
Copy link
Contributor Author

I tried to just extend that test but it didn't work well due to constexpr steps limit

😈 Should we increase the limit?

Unfortunately clang-cl does not recognize that option 🙁

@CaseyCarter CaseyCarter added the cxx23 C++23 feature label Aug 23, 2022
stl/inc/charconv Outdated Show resolved Hide resolved
Copy link
Member

@CaseyCarter CaseyCarter left a comment

Choose a reason for hiding this comment

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

Just a comment nitpick that I'll apply.

tests/std/tests/P0067R5_charconv/test.cpp Outdated Show resolved Hide resolved
stl/inc/charconv Outdated Show resolved Hide resolved
stl/inc/charconv Outdated Show resolved Hide resolved
tests/std/tests/P0067R5_charconv/test.cpp Outdated Show resolved Hide resolved
@StephanTLavavej
Copy link
Member

Thanks, this looks great! 😻 I pushed small changes for an unnecessary cast and tiny style improvements (FYI @CaseyCarter @strega-nil-ms).

The test's use of internal machinery (_CONSTEXPR23, _Is_constant_evaluated()) is reasonable - there is precedent (we have a few test uses of _CONSTEXPR20), and they'll be easy to address if in the moderate-far future we add Modules configurations to all tests (as import std; will not provide such machinery).

@@ -197,9 +197,11 @@ void test_common_to_chars(
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess we can avoid using internal mechanisms in test file by replacing them with the following... (no change requested)

Suggested change
#if _HAS_CXX23 // testing constexpr overloads for integral types in C++23 (P2291R3)
#define CONSTEXPR23 constexpr
constexpr is_runtime_evaluated() noexcept {
return !is_constant_evaluated();
}
#else // ^^^ _HAS_CXX23 / vvv !_HAS_CXX23
#define CONSTEXPR23 inline
constexpr is_runtime_evaluated() noexcept {
return true;
}
#endif // _HAS_CXX23

Copy link
Member

Choose a reason for hiding this comment

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

Yes, we could do something like that in the future, possibly centralized in a lightweight classic header.

For the time being, tests don't need to strenuously avoid internal machinery and macros as long as there is reasonable justification.

@StephanTLavavej StephanTLavavej self-assigned this Aug 26, 2022
@StephanTLavavej
Copy link
Member

I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

@StephanTLavavej StephanTLavavej merged commit 473245c into microsoft:main Aug 27, 2022
@StephanTLavavej
Copy link
Member

Thanks for making <charconv> even more powerful! 🚀 🧮 😻

@AdamBucior AdamBucior deleted the constexpr-charconv branch August 27, 2022 07:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cxx23 C++23 feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

P2291R3 constexpr Integral <charconv>
6 participants