[libcxx-commits] [PATCH] D131218: [libc++] Implement P2417R2 (A more constexpr bitset)

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik marked 2 inline comments as done.
philnik added inline comments.


================
Comment at: libcxx/include/__config:853
+#  if _LIBCPP_STD_VER > 20
+#    define _LIBCPP_CONSTEXPR_CXX23 constexpr
+#  else
----------------
jloser wrote:
> 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.
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".


================
Comment at: libcxx/test/std/utilities/template.bitset/bitset.members/right_shift_eq.pass.cpp:53
+#if TEST_STD_VER >= 23
+  static_assert(test_right_shift<0>());
+  static_assert(test_right_shift<1>());
----------------
jloser wrote:
> 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?
`test_right_shift<1000>()` reaches the constexpr step limit. The other tests combined do the same thing.


================
Comment at: libcxx/test/std/utilities/template.bitset/bitset.members/to_string.pass.cpp:130
+  static_assert(test_to_string<64>());
+  static_assert(test_to_string<65>());
+#endif
----------------
jloser wrote:
> Similar comment about a reusable test function and why can't we test `test_to_string<1000>()` in `constexpr` branch?
And the same answer.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131218/new/

https://reviews.llvm.org/D131218