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

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added inline comments.


================
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,
----------------
Mordante wrote:
> Can you add constexpr to all the signatures in the synopsis.
AFAIK we normally don't do that when we have `// constexpr since C++ab`.


================
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
----------------
Mordante wrote:
> This one already was constexpr, or wast that changed in C++23 too?
Oops, that one was already constexpr.


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


================
Comment at: libcxx/test/std/utilities/template.bitset/bitset.members/op_xor_eq.pass.cpp:54
+  static_assert(test_op_xor_eq<65>());
+#endif
 
----------------
Mordante wrote:
> Can you improve this to use the typical libc++ constexpr test.
No, in this case combining them also hits the constexpr limit.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131218