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.