Details
- Reviewers
- ldionne - Mordante - var-const 
- Group Reviewers
- Restricted Project 
- Commits
- rGf02120fba28f: [libc++] Implement P2417R2 (A more constexpr bitset)
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
| libcxx/include/__config | ||
|---|---|---|
| 846 | 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 | ||
| 68 | 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? | |
| libcxx/include/__config | ||
|---|---|---|
| 846 | 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 | ||
| 68 | 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 | ||
|---|---|---|
| 846 | Thanks for discussing this on Discord. | |
| libcxx/include/bitset | ||
| 43 | Can you add constexpr to all the signatures in the synopsis. | |
| 68–72 | 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. | |
| 25–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? 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. | |
- Address comments
| libcxx/include/bitset | ||
|---|---|---|
| 43 | AFAIK we normally don't do that when we have // constexpr since C++ab. | |
| 68–72 | 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. | |
LGTM with my comments.
| libcxx/include/__config | ||
|---|---|---|
| 846 | 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 | 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. | 
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.