[libcxx-commits] [PATCH] D110718: [libc++] Implement P1391

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

Quuxplusone requested changes to this revision.
Quuxplusone added a subscriber: cor3ntin.
Quuxplusone added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/include/string_view:88-89
       constexpr basic_string_view(const charT* str, size_type len);
+      template <class It, class End>
+      constexpr basic_string_view(It begin, End end); // C++20
 
----------------
@cor3ntin: The Tony Table at the beginning of http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p1391r4.pdf claims that after this paper, `foo(vec)` will compile... but that's just plain false, correct? IIUC, the actual achievement of p1391 was to make `foo(vec.begin(), vec.end())` compile; and then http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2021/p1989r2.pdf is the one that would actually support `foo(vec)`?
Just checking, because if p1391 isn't wrong, then I'm super confused.


================
Comment at: libcxx/include/string_view:285
+    constexpr _LIBCPP_INLINE_VISIBILITY basic_string_view(_It __begin, _End __end)
+       : __data(_VSTD::to_address(__begin)), __size(static_cast<size_type>(_VSTD::distance(__begin, __end)))
+    {
----------------
`__size(__end - __begin)`, please. We know the expression is well-formed because we know `_End` is a sized sentinel for `_It`.


================
Comment at: libcxx/include/string_view:287
+    {
+        _LIBCPP_ASSERT(__begin <= __end, "invalid range in string_view's constructor (iterator, iterator)");
+    }
----------------
Vice versa, I see no reason for `__begin <= __end` to be valid. Please add a test case using a non-iterator sentinel type. (I think my SFINAE test below is //not// sufficient, because it won't instantiate the body of this template, and it's the //body// that explodes.)


================
Comment at: libcxx/test/std/strings/string.view/string.view.cons/from_iterator_iterator.pass.cpp:43
+  return 0;
+}
+
----------------
This test file should probably be named `from_iterator_sentinel.pass.cpp`.
I'd also like to see some SFINAE tests, and some mixed-constness tests. Just these would be fine:
```
static_assert( std::is_constructible_v<std::string_view, const char*, char*>); // OK
static_assert( std::is_constructible_v<std::string_view, char*, const char*>); // OK
static_assert(!std::is_constructible_v<std::string_view, char*, void*>); // not a sentinel
static_assert(!std::is_constructible_v<std::string_view, signed char*, signed char*>); // wrong char type
static_assert(!std::is_constructible_v<std::string_view, random_access_iterator<char*>, random_access_iterator<char*>>); // not contiguous
static_assert( std::is_constructible_v<std::string_view, contiguous_iterator<char*>, contiguous_iterator<char*>>); // OK
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110718