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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision.
ldionne added a comment.
This revision is now accepted and ready to land.

LGTM with my comments.



================
Comment at: libcxx/include/__config:853
+#  if _LIBCPP_STD_VER > 20
+#    define _LIBCPP_CONSTEXPR_CXX23 constexpr
+#  else
----------------
jloser wrote:
> Mordante wrote:
> > philnik wrote:
> > > 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".
> > Thanks for discussing this on Discord.
> Thanks for the explanation/rationale and discussion on Discord. I'm +1 for this naming now.
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`.


================
Comment at: libcxx/test/std/utilities/template.bitset/bitset.members/flip_all.pass.cpp:46
+  test();
+  test_flip_all<1000>();
+#if TEST_STD_VER >= 23
----------------
This also applies elsewhere in this patch.


================
Comment at: libcxx/test/std/utilities/template.bitset/bitset.members/set_one.out_of_range.pass.cpp:11
 
-// test bitset<N>& set(size_t pos, bool val = true);
+// test bitset<N>& set(size_t pos, bool val = true); // constexpr since C++23
 
----------------
Not mandatory, but we could do this while we're at it, since we touch all these lines.


================
Comment at: libcxx/test/std/utilities/template.bitset/bitset.members/test.pass.cpp:9
 
 // test constexpr bool test(size_t pos) const;
 
----------------



================
Comment at: libcxx/test/std/utilities/template.bitset/bitset.operators/stream_in.pass.cpp:15
 // basic_istream<charT, traits>&
-// operator>>(basic_istream<charT, traits>& is, bitset<N>& x);
+// operator>>(basic_istream<charT, traits>& is, bitset<N>& x); // constexpr since C++23
 
----------------
I don't think so?


================
Comment at: libcxx/test/std/utilities/template.bitset/bitset.operators/stream_out.pass.cpp:15
 // basic_ostream<charT, traits>&
-// operator<<(basic_ostream<charT, traits>& os, const bitset<N>& x);
+// operator<<(basic_ostream<charT, traits>& os, const bitset<N>& x); // constexpr since C++23
 
----------------
Same here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131218