[libcxx-commits] [PATCH] D153201: [libc++] Adds string_view constructor overload

[Louis Dionne via Phabricator via libcxx-commits]

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

Thanks! LGTM w/ test comments applied.



================
Comment at: libcxx/include/bitset:42-46
     template <class charT>
-        explicit bitset(const charT* str,
-                        typename basic_string<charT>::size_type n = basic_string<charT>::npos,
-                        charT zero = charT('0'), charT one = charT('1')); // constexpr since C++23
+        constexpr explicit bitset(
+            const charT* str,
+            typename basic_string_view<charT>::size_type n = basic_string_view<charT>::npos, // s/string/string_view since C++26
+            charT zero = charT('0'), charT one = charT('1'));                                // constexpr since C++23
----------------
Maybe this is more explicit?

```
template <class charT>
        constexpr explicit bitset(const charT* str,
            typename basic_string<charT>::size_type n = basic_string<charT>::npos,
            charT zero = charT('0'), charT one = charT('1'));                                // until C++26, constexpr since C++23

template <class charT>
        constexpr explicit bitset(const charT* str,
            typename basic_string_view<charT>::size_type n = basic_string_view<charT>::npos,
            charT zero = charT('0'), charT one = charT('1'));                                // since C++26
```


================
Comment at: libcxx/test/std/utilities/template.bitset/bitset.cons/string_view_ctor.pass.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
We need to add stricter tests for the default arguments. Specifically, we should be calling as

```
bitset(s)
bitset(s, pos)
bitset(s, pos, n)
bitset(s, pos, n, zero)
bitset(s, pos, n, zero, one)
```

Also, it would be great to backport these additional tests to the string and c-string constructors, since it'll be trivial to do that. I'd do it as a fly-by change in this patch.


================
Comment at: libcxx/test/std/utilities/template.bitset/bitset.cons/string_view_ctor.pass.cpp:3
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
----------------
We are also not testing for the `explicit`-ness of the constructor. We can using the `!is_convertible && is_constructible` check.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153201