This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Implements constexpr <charconv>.
ClosedPublic

Authored by Mordante on Aug 6 2022, 1:55 AM.

Details

Reviewers
ldionne
Group Reviewers
Restricted Project
Commits
rGa1e13a80d06c: [libc++] Implements constexpr <charconv>.
Summary

Implements:

  • P2291R3 Add Constexpr Modifiers to Functions to_chars and from_chars for Integral Types in <charconv> Header

Diff Detail

Event Timeline

Mordante created this revision.Aug 6 2022, 1:55 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 6 2022, 1:55 AM
Herald added a subscriber: arichardson. · View Herald Transcript
Mordante requested review of this revision.Aug 6 2022, 1:55 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 6 2022, 1:55 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
philnik added a subscriber: philnik.Aug 6 2022, 3:12 AM
philnik added inline comments.
libcxx/include/__charconv/tables.h
26

What is the difference between the versions? Why can't the arrays be constexpr before C++17?

libcxx/include/__config
852–856 ↗(On Diff #450499)

I've named this _LIBCPP_CONSTEXPR_CXX23 in D131218 because I find it a lot more intuitive. I'll open a discussion on Discord whether we want this change or not.

libcxx/include/charconv
33–34

I think we don't add the constexpr if it's "constexpr since C++ab".

711–720

What does this do here?

742

Is there a reason this isn't in C++11 or 14?

libcxx/test/support/charconv_test_helpers.h
99

Maybe make this a runtime-only test or implement it a constexpr memcmp for portability?

Mordante updated this revision to Diff 450510.Aug 6 2022, 4:16 AM

Fixes CI.

Mordante updated this revision to Diff 450511.Aug 6 2022, 4:26 AM

More CI fixes.

Mordante updated this revision to Diff 450516.Aug 6 2022, 5:06 AM

Attempts to fix the CI.

Mordante updated this revision to Diff 450525.Aug 6 2022, 5:55 AM

CI fixes.

Mordante updated this revision to Diff 450816.Aug 8 2022, 8:28 AM

Attempts to fix GCC CI.

Mordante updated this revision to Diff 450837.Aug 8 2022, 9:19 AM

Fixes GCC CI.

Mordante marked 3 inline comments as done.Aug 8 2022, 9:29 AM

Thanks for the review!

libcxx/include/__charconv/tables.h
26

No inline constexpr before C++17. I want to look at this later but prefer to move that out of this change.

libcxx/include/__config
852–856 ↗(On Diff #450499)

Yeah it seems we manage to get 3 reviews that basically add this macro at the same time.
I keep it for now until we finished the discussion which direction to go.

libcxx/include/charconv
33–34

We do, git grep 'constexpr.*constexpr' include shows a lot of hits, some for C++14.

711–720

The "script" to generate the table below.

742

yes not inline constexpr and log2f itself isn't constexpr so it can't be used in C++23.

libcxx/test/libcxx/clang_tidy.sh.cpp
11 ↗(On Diff #450837)

@philnik does running this in GCC have any benefit for this test?
I see there are work-arounds for unsupported diagnostics and now I have an unsupported compiler option -fconstexpr-ops-limit. I even wonder should this be limited to the clang compiler?

philnik added inline comments.Aug 8 2022, 10:27 AM
libcxx/include/__charconv/tables.h
26

Isn't this in the class to achieve the same effect? Clang seems to be happy with that in C++11: https://godbolt.org/z/c6nz39xbv

libcxx/include/__config
852–856 ↗(On Diff #450499)

Sounds good to me. Although there seems to be no opposition to the change.

libcxx/include/charconv
711–720

Oh OK. Could you maybe add a comment like // __from_chars_log2f_lut generator script?

libcxx/test/libcxx/clang_tidy.sh.cpp
11 ↗(On Diff #450837)

It also works for people who primarily use GCC, but I guess that's about zero people. So it's probably fine to disable it entirely for GCC. Could you also remove the compatibility stuff in that case?

Mordante planned changes to this revision.Aug 8 2022, 11:38 AM
Mordante marked 3 inline comments as done.
Mordante added inline comments.
libcxx/include/__config
852–856 ↗(On Diff #450499)

True, but I expect one of the other two patches to land before this one, so then I can remove this hunk, rebase, and fix the other places. There are some changes here that shouldn't be hiding in this review; but I really want to get a CI run before making these changes.

libcxx/test/libcxx/clang_tidy.sh.cpp
11 ↗(On Diff #450837)

Yes I intend to do that, however I want to move these changes to their own review.

h-vetinari added inline comments.
libcxx/include/__config
852–856 ↗(On Diff #450499)

To me, _LIBCPP_CONSTEXPR_AFTER_CXX20 sounds like it is inclusive "after", i.e. also C++20.

Perhaps "FROM" is less ambiguous w.r.t to >= vs. > than "AFTER"? I think the most intuitive option might be: _LIBCPP_CONSTEXPR_FROM_CXX23

Mordante added inline comments.Aug 10 2022, 9:27 AM
libcxx/include/__config
852–856 ↗(On Diff #450499)

To me after sounds exclusive. I'm not sure when _LIBCPP_CONSTEXPR_AFTER_CXX11 was added, but I strongly expect that was in the era where C++0x had just slipped to C++11 and it was uncertain the then new 3 year schedule would be a real thing. So I understand the naming from a historical point of view. I prefer without the FROM since that's what we already do in our lit tests. We already settled on that in our Discord channel.

Mordante marked 6 inline comments as done.Oct 8 2022, 4:55 AM
Mordante added inline comments.
libcxx/include/__charconv/tables.h
26

Changed to inline constexpr instead.

Mordante updated this revision to Diff 466276.Oct 8 2022, 4:56 AM
Mordante marked an inline comment as done.

Reworked the patch to work on main and addressed review comments.

Mordante updated this revision to Diff 466277.Oct 8 2022, 5:05 AM

Fixes CI.

Mordante updated this revision to Diff 466278.Oct 8 2022, 5:32 AM

Attempts to fix CI.

Mordante updated this revision to Diff 466283.Oct 8 2022, 6:20 AM

Attempts to fix CI.

Mordante updated this revision to Diff 466375.Oct 9 2022, 8:13 AM

Fix the CI.

Mordante updated this revision to Diff 466703.Oct 10 2022, 10:54 PM

Removes pre C++17 support.

ldionne accepted this revision.Oct 11 2022, 9:45 AM
ldionne added a subscriber: ldionne.
ldionne added inline comments.
libcxx/include/charconv
741–742

Do we ever validate that __base <= 36? If not, we should add an assertion, but probably at a higher level. And here we should just add a comment like // __base is always between 2 and 36 inclusive.

libcxx/test/support/charconv_test_helpers.h
99

I think we could use std::equal here?

104–105

Can we get rid of this while we're at it?

140

This does weaken the test inside constexpr because this is now a roundtrip test (and we already have a test for that), but I guess it's better than nothing.

This revision is now accepted and ready to land.Oct 11 2022, 9:45 AM
Mordante marked 4 inline comments as done.Oct 11 2022, 10:08 AM

Thanks for the reviews!

libcxx/include/charconv
797

The base range validation is done here.

Mordante updated this revision to Diff 466851.Oct 11 2022, 10:14 AM

Rebased and addresses review comments.

This revision was automatically updated to reflect the committed changes.