This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Implement P2417R2 (A more constexpr bitset)
ClosedPublic

Authored by philnik on Aug 4 2022, 4:15 PM.

Details

Diff Detail

Event Timeline

philnik created this revision.Aug 4 2022, 4:15 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 4 2022, 4:15 PM
philnik requested review of this revision.Aug 4 2022, 4:15 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 4 2022, 4:15 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
jloser added a subscriber: jloser.Aug 4 2022, 4:26 PM
jloser added inline comments.
libcxx/include/__config
853

I'd prefer _LIBCPP_CONSTEXPR_AFTER_CXX20 for consistency and presumably std::bitset will be constexpr still when C++26 is around, so the macro name won't be great at that time.

libcxx/test/std/utilities/template.bitset/bitset.members/right_shift_eq.pass.cpp
53

What about test_right_shift<1000>() in constexpr?

It seems a simpler refactoring is to put the calls into a reusable function, say test and then do

test()
static_assert(test())

What do you think?

libcxx/test/std/utilities/template.bitset/bitset.members/to_string.pass.cpp
130

Similar comment about a reusable test function and why can't we test test_to_string<1000>() in constexpr branch?

philnik marked 2 inline comments as done.Aug 4 2022, 4:36 PM
philnik added inline comments.
libcxx/include/__config
853

I want to avoid the mental burden of forming AFTER_CXX20 to CXX23. I'd actually prefer it a lot if we renamed the other macros. It's also the same naming scheme that we use in the tests. I don't really see how AFTER_CXXab is a better name when we have C++26. IMO it's pretty obvious that _LIBCPP_CONSTEXPR_CXX23 is "it's constexpr since C++23".

libcxx/test/std/utilities/template.bitset/bitset.members/right_shift_eq.pass.cpp
53

test_right_shift<1000>() reaches the constexpr step limit. The other tests combined do the same thing.

libcxx/test/std/utilities/template.bitset/bitset.members/to_string.pass.cpp
130

And the same answer.

Thanks for working on this!

libcxx/include/__config
853

Thanks for discussing this on Discord.

libcxx/include/bitset
43

Can you add constexpr to all the signatures in the synopsis.

68

This one already was constexpr, or wast that changed in C++23 too?

88

Not yours, but N is used in the synopsis for these two operators.

libcxx/test/std/utilities/template.bitset/bitset.cons/char_ptr_ctor.pass.cpp
10

Please update the synopsis and validate the other tests too.

26

How about moving this in the #ifndef TEST_HAS_NO_EXCEPTIONS block instead?

libcxx/test/std/utilities/template.bitset/bitset.members/count.pass.cpp
48

Why can't this one be tested constexpr? Due to the default constexpr limit in Clang?
Can you add a comment with the reason?

Do we really need to test 1000 or would something like 513 also suffice?

libcxx/test/std/utilities/template.bitset/bitset.members/index_const.pass.cpp
64

Please move above to be consistent with the other tests.

libcxx/test/std/utilities/template.bitset/bitset.members/op_xor_eq.pass.cpp
54

Can you improve this to use the typical libc++ constexpr test.

jloser added inline comments.Aug 9 2022, 7:29 AM
libcxx/include/__config
853

Thanks for the explanation/rationale and discussion on Discord. I'm +1 for this naming now.

libcxx/test/std/utilities/template.bitset/bitset.members/right_shift_eq.pass.cpp
53

Got it - thanks for clarifying.

philnik updated this revision to Diff 451644.Aug 10 2022, 3:00 PM
philnik marked 13 inline comments as done.
  • Address comments
libcxx/include/bitset
43

AFAIK we normally don't do that when we have // constexpr since C++ab.

68

Oops, that one was already constexpr.

libcxx/test/std/utilities/template.bitset/bitset.members/count.pass.cpp
48

The 1000 is special-cased in get_test_cases. I don't think adding a comment makes a lot of sense. The exact same comment would have to be added to almost every bitset test. I think it's especially not useful because the first guess of most people is the correct one.

libcxx/test/std/utilities/template.bitset/bitset.members/op_xor_eq.pass.cpp
54

No, in this case combining them also hits the constexpr limit.

ldionne accepted this revision.Aug 11 2022, 9:58 AM

LGTM with my comments.

libcxx/include/__config
853

I know this has been discussed, but let's use _AFTER_CXX20 until we actually do the rename. I also don't think this will end up being _LIBCPP_CONSTEXPR_CXX23, but probably something like _LIBCPP_CONSTEXPR_IN_CXX23 or _LIBCPP_CONSTEXPR_SINCE_CXX23.

libcxx/test/std/utilities/template.bitset/bitset.members/flip_all.pass.cpp
46

This also applies elsewhere in this patch.

libcxx/test/std/utilities/template.bitset/bitset.members/set_one.out_of_range.pass.cpp
11 ↗(On Diff #451644)

Not mandatory, but we could do this while we're at it, since we touch all these lines.

libcxx/test/std/utilities/template.bitset/bitset.members/test.pass.cpp
9
libcxx/test/std/utilities/template.bitset/bitset.operators/stream_in.pass.cpp
15 ↗(On Diff #451644)

I don't think so?

libcxx/test/std/utilities/template.bitset/bitset.operators/stream_out.pass.cpp
15 ↗(On Diff #451644)

Same here.

This revision is now accepted and ready to land.Aug 11 2022, 9:58 AM
philnik updated this revision to Diff 452412.Aug 13 2022, 4:52 AM
philnik marked 6 inline comments as done.
  • Address comments
This revision was landed with ongoing or failed builds.Aug 14 2022, 1:34 AM
This revision was automatically updated to reflect the committed changes.