[libcxx-commits] [PATCH] D113161: [libc++] Implement P1989R2: range constructor for string_view

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone accepted this revision as: Quuxplusone.
Quuxplusone added inline comments.


================
Comment at: libcxx/include/string_view:291
     template <contiguous_iterator _It, sized_sentinel_for<_It> _End>
-      requires (same_as<iter_value_t<_It>, _CharT> && !convertible_to<_End, size_type>)
+      requires (is_same_v<iter_value_t<_It>, _CharT> && !is_convertible_v<_End, size_type>)
     constexpr _LIBCPP_HIDE_FROM_ABI basic_string_view(_It __begin, _End __end)
----------------
:+1:
I idly wonder if it's possible to make a (contrived) test case that regression-tests this. There's https://stackoverflow.com/a/62644127/1424877 but it works only for conversions to user-defined class types, not to `size_t`.


================
Comment at: libcxx/include/string_view:314
+      )
+    constexpr _LIBCPP_HIDE_FROM_ABI basic_string_view(_Range&& __r) : __data(ranges::data(__r)), __size(ranges::size(__r)) {}
+#endif
----------------
Linebreak before `basic_string_view`, and perhaps also before `:` if the line length is still an issue. (Compare line 293.)


================
Comment at: libcxx/test/std/strings/string.view/string.view.cons/from_range.pass.cpp:83
+static_assert(std::is_constructible_v<std::string_view, const std::vector<char>&>); // contiguous sized range const lvalue
+static_assert(std::is_constructible_v<std::string_view, std::vector<char>&&>); // contiguous sized range rvalue
+
----------------
Might as well test `const std::vector<char>&&` while you're here. (And I'd remove the end-of-line comments; they don't add anything.)


================
Comment at: libcxx/test/std/strings/string.view/string.view.cons/from_range.pass.cpp:85
+
+using NonContiguousRange = std::ranges::subrange<random_access_iterator<char*>>;
+static_assert(!std::ranges::contiguous_range<NonContiguousRange>);
----------------
Suggest `SizedButNotContiguousRange`, to match the pattern set by line 90.


================
Comment at: libcxx/test/std/strings/string.view/string.view.cons/from_range.pass.cpp:110-111
+struct WithTraitsType {
+  char* begin() { return nullptr; }
+  char* end() { return nullptr; }
+  using traits_type = CharTraits;
----------------
Since `begin` and `end` are never called, you can omit their bodies.
Personally I'd also mark them `const`, just to prove that their non-const-ness is not related to the point of this test.


================
Comment at: libcxx/test/std/strings/string.view/string.view.cons/from_range.pass.cpp:115
+
+static_assert(!std::is_constructible_v<std::string_view, WithTraitsType<std::char_traits<char16_t>>>); // wrong traits_type
+
----------------
Either make `WithTraitsType` a non-template, or use its templateyness to prove that `string_view` //is// constructible with the //correct// traits type:
```
#include "constexpr_char_traits.h"  // We have no "really weird char traits" helper header? Darn.
using CCT = constexpr_char_traits<char>;
static_assert(std::is_constructible_v<std::string_view, WithTraitsType<std::char_traits<char>>>);
static_assert(std::is_constructible_v<std::wstring_view, WithTraitsType<std::char_traits<wchar_t>>>);
static_assert(std::is_constructible_v<std::basic_string_view<char, CCT>, WithTraitsType<CCT>>);
static_assert(!std::is_constructible_v<std::string_view, WithTraitsType<CCT>>);  // wrong traits type
static_assert(!std::is_constructible_v<std::wstring_view, WithTraitsType<std::char_traits<char>>>);  // wrong traits type
```
Here I'm also drive-by testing that we're looking at the whole traits type, not just its character type.


================
Comment at: libcxx/test/std/strings/string.view/string.view.cons/from_range.pass.cpp:108
+    operator std::string_view() const noexcept = delete;
+  };
+
----------------
jloser wrote:
> Quuxplusone wrote:
> > Please remove the `noexcept`. Also, let's "Don't inherit from `std` types" here. I think all you need to do is
> > ```
> > struct DeletedStringViewConversionOperator {
> >     const char *data_ = "test";
> >     constexpr const char *begin() const { return data_; }
> >     constexpr const char *end() const { return data_ + 4; }
> >     operator std::string_view() const = delete;
> > };
> > DeletedStringViewConversionOperator d;
> > std::string_view sv = d;
> > assert(sv == "test");
> > std::string_view csv = std::as_const(d);
> > assert(csv == "test");
> > ```
> > Bonus: this should be constexpr-friendly out of the box.
> > 
> > It might also be worth testing a similar type with a deleted but //non//-const-qualified `operator string_view`. In that case, I'm having trouble visualizing it, but maybe the effect would be that `sv = d` would be noisily ill-formed and `csv = std::as_const(d)` would be well-formed?
> > std::string_view csv = std::as_const(d); 
> > assert(csv == "test");
> 
> That `string_view` construction results in a hard error since we'd be invoking the deleted conversion operator. I've changed the tests to cover both cases without hard erroring:
> 
> ```
> struct DeletedStringViewConversionOperator {
>     const char *data_ = "test";
>     constexpr const char *begin() const { return data_; }
>     constexpr const char *end() const { return data_ + 4; }
>     operator std::string_view() = delete;
> };
> 
> struct DeletedConstStringViewConversionOperator {
>     const char *data_ = "test";
>     constexpr const char *begin() const { return data_; }
>     constexpr const char *end() const { return data_ + 4; }
>     operator std::string_view() const = delete;
> };
> 
>     DeletedStringViewConversionOperator d;
>     std::string_view csv = std::as_const(d);
>     assert(csv == "test");
>     
>     DeletedConstStringViewConversionOperator dc;
>     std::string_view sv = dc;
>     assert(sv == "test");
> ```
SGTM.


================
Comment at: libcxx/test/std/strings/string.view/string.view.deduct/range.pass.cpp:8
 //===----------------------------------------------------------------------===//
-// UNSUPPORTED: c++03, c++11, c++14, c++17
+// UNSUPPORTED: c++03, c++11, c++14, c++17, c++20
 // UNSUPPORTED: libcpp-no-concepts
----------------
IIUC, Phab is confused here: you're not //actually// removing any test coverage here, you're just adding a second copy of this file. Right?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113161