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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

Thanks for working on this!



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


================
Comment at: libcxx/include/bitset:43
     template <class charT>
         explicit bitset(const charT* str,
                         typename basic_string<charT>::size_type n = basic_string<charT>::npos,
----------------
Can you add constexpr to all the signatures in the synopsis.


================
Comment at: libcxx/include/bitset:68
     // element access:
-    constexpr bool operator[](size_t pos) const; // for b[i];
-    reference operator[](size_t pos);            // for b[i];
-    unsigned long to_ulong() const;
-    unsigned long long to_ullong() const;
-    template <class charT, class traits, class Allocator>
+    constexpr bool operator[](size_t pos) const; // constexpr since C++23
+    reference operator[](size_t pos);            // constexpr since C++23
----------------
This one already was constexpr, or wast that changed in C++23 too?


================
Comment at: libcxx/include/bitset:88
+    bitset operator<<(size_t pos) const noexcept;      // constexpr since C++23
+    bitset operator>>(size_t pos) const noexcept;      // constexpr since C++23
 };
----------------
Not yours, but `N` is used in the synopsis for these two operators.


================
Comment at: libcxx/test/std/utilities/template.bitset/bitset.cons/char_ptr_ctor.pass.cpp:10
 // template <class charT>
 //     explicit bitset(const charT* str,
 //                     typename basic_string<charT>::size_type n = basic_string<charT>::npos,
----------------
Please update the synopsis and validate the other tests too.


================
Comment at: libcxx/test/std/utilities/template.bitset/bitset.cons/char_ptr_ctor.pass.cpp:26
 {
-    {
+  if (!TEST_IS_CONSTANT_EVALUATED) {
 #ifndef TEST_HAS_NO_EXCEPTIONS
----------------
How about moving this in the `#ifndef TEST_HAS_NO_EXCEPTIONS` block instead?


================
Comment at: libcxx/test/std/utilities/template.bitset/bitset.members/count.pass.cpp:48
+  test();
+  test_count<1000>();
+#if TEST_STD_VER >= 23
----------------
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?


================
Comment at: libcxx/test/std/utilities/template.bitset/bitset.members/index_const.pass.cpp:64
+#endif
+  test_index_const<1000>();
+
----------------
Please move above to be consistent with the other tests.


================
Comment at: libcxx/test/std/utilities/template.bitset/bitset.members/op_xor_eq.pass.cpp:54
+  static_assert(test_op_xor_eq<65>());
+#endif
 
----------------
Can you improve this to use the typical libc++ constexpr test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131218