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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
Comment at: libcxx/include/string_view:302
+      requires (
+        !is_same_v<remove_cvref_t<_Range>, basic_string_view> &&
+        ranges::contiguous_range<_Range> &&
----------------
I'm surprised this isn't written in the spec. I assume this is necessary to break ambiguities in case you try to construct a `string_view` from a `string_view const&&` or a `string_view&`? If you construct from a `string_view const&`/`string_view&&`, the copy/move constructor respectively should be preferred, but not for their `&` and `const&&` counterparts. Is my understanding correct?

Also, do we have a test that breaks if you remove that constraint?


================
Comment at: libcxx/include/string_view:306
+        is_same_v<ranges::range_value_t<_Range>, _CharT> &&
+        (!is_convertible_v<_Range, const _CharT*>) &&
+        (!requires(remove_cvref_t<_Range>& d) {
----------------



================
Comment at: libcxx/test/std/strings/string.view/string.view.cons/from_range.pass.cpp:44
+
+constexpr void test_deleted_string_view_conversion_operator() {
+  struct DeletedStringViewConversionOperator {
----------------
IMO, trying to encode what this test is doing in the function name is the worst of both worlds:

1. You can't use plain english to describe what the test really does in detail, and
2. The function name is long, which makes it really hard to read (especially when there are multiple long identifiers bunched together, I always end up having to re-read really slowly to figure out which one is being referred to -- here you have only one).

IMO, this would be better, directly inside `test()` below:

```
constexpr bool test() {
  test<char>();
#ifndef TEST_HAS_NO_WIDE_CHARACTERS
  test<wchar_t>();
#endif
  test<char8_t>();
  test<char16_t>();
  test<char32_t>();

  // Test that we're not trying to use the type's conversion operator to string_view in the constructor.
  {
    DeletedStringViewConversionOperator d;
    std::string_view csv = std::as_const(d);
    assert(csv == "test");

    DeletedConstStringViewConversionOperator dc;
    std::string_view sv = dc;
    assert(sv == "test");
  }

  return true;
}
```

Also, I think this would be slightly clearer, simply because we don't use `as_const` a lot in the test suite -- but I do find there's something nice to doing it your way (you could reuse the same variable for const and non-const tests if that was relevant):

```
DeletedStringViewConversionOperator const d;
std::string_view csv = d;
assert(csv == "test");
```

Finally, I would pin down the types of variables instead of using deduction guides, so I'd use `std::string_view<char> sv` instead of `std::string_view sv` -- this makes it obvious that any potential CTAD has nothing to do with what we're trying to test.


Those are just suggestions, but the one about adding a comment instead of using a lengthy function name is something I'd love if you could address.



================
Comment at: libcxx/test/std/strings/string.view/string.view.cons/from_range.pass.cpp:158
+  static_assert(test());
+  test_throwing(); // clang thinks data() member function is invalid constexpr due to use of throw, so we can't static_assert with this.
+
----------------
Clang is correct to not let you do this -- I think the comment adds very little information. WDYT?


================
Comment at: libcxx/test/std/strings/string.view/string.view.deduct/range.pass.cpp:24
 constexpr void test() {
-  auto val = MAKE_STRING_VIEW(CharT, "test");
-  auto sv = std::basic_string_view(val.begin(), Sentinel(val.end()));
+  auto val = MAKE_STRING(CharT, "test");
+  auto sv = std::basic_string_view(val);
----------------
Can we add a test that exercises the deduction guide from a range that is *not* a `std::string`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113161