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

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

Quuxplusone added inline comments.


================
Comment at: libcxx/test/std/strings/string.view/string.view.cons/from_range.pass.cpp:60
+  DeletedStringViewConversionOperator d;
+  std::string_view csv = std::as_const(d);
+  assert(csv == "test");
----------------
Mordante wrote:
> jloser wrote:
> > Mordante wrote:
> > > Can you test whether it's !is_constructible, when the not used as const?
> > I think that's a bit tricky. For example,
> > 
> > ```
> > static_assert(!std::is_constructible_v<std::string_view, decltype(d)>); // fails oddly enough
> > ```
> > 
> > but
> > ```
> > std::string_view s = d; // hard error
> > assert(s == "test");
> > ```
> > 
> > hard errors when trying to construct the `std::string_view` since it invokes the deleted implicit conversion operator:
> > 
> > ```
> > libcxx/test/std/strings/string.view/string.view.cons/from_range.pass.cpp:66:20: error: conversion function from 'DeletedStringViewConversionOperator' to 'std::string_view' (aka 'basic_string_view<char>') invokes a deleted function
> >   std::string_view s = d;
> > ```
> > 
> > Did you have something specifically in mind that you think could work?
> Interesting, this is the test I expected to work...
I'm not sure I'm parsing either of your comments correctly, but yeah I would have expected it to be simple to add these lines to this test:
```
static_assert(!std::is_constructible_v<std::string_view, DeletedStringViewConversionOperator>);
static_assert(std::is_constructible_v<std::string_view, const DeletedStringViewConversionOperator>);
static_assert(std::is_constructible_v<std::string_view, DeletedConstStringViewConversionOperator>);
static_assert(!std::is_constructible_v<std::string_view, const DeletedConstStringViewConversionOperator>);
```
@jloser, are you saying that one of those lines gives a hard error? I don't //think// it should.

Also, naming nit: the words `StringView` aren't pulling their weight and could be omitted: `DeletedConversionOperator`, `DeletedConstConversionOperator`.


================
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 {
----------------
ldionne wrote:
> 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.
> 
> I'd use `std::string_view<char> sv` instead of `std::string_view sv`

(Brain fart there: `string_view` is the non-template, `basic_string_view<char>` is the template. :))


================
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.
+
----------------
ldionne wrote:
> Clang is correct to not let you do this -- I think the comment adds very little information. WDYT?
+1.


================
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);
----------------
ldionne wrote:
> Can we add a test that exercises the deduction guide from a range that is *not* a `std::string`?
Something like this could do the trick:
```
#include "test_iterators.h"
struct Widget {
    char16_t *data_ = u16"foo";
    contiguous_iterator<const char16_t*> begin() const { return data_; }
    contiguous_iterator<const char16_t*> end() const { return data_ + 3; }
};
basic_string_view bsv = Widget();
ASSERT_SAME_TYPE(decltype(bsv), std::basic_string_view<char16_t>);
```


================
Comment at: libcxx/test/std/strings/string.view/string.view.deduct/range.pass.cpp:43
 int main(int, char**) {
   test();
 
----------------
Is the lack of `static_assert(test());` here an accidental oversight? I see a lot of `constexpr` above that doesn't make sense otherwise.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113161