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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

I really would like to understand why the test fails before approving.
I might want to have a look at it myself, because I'm quite curious why it doesn't work.



================
Comment at: libcxx/include/type_traits:1417-1418
 
+template<class _Tp> struct __type_identity { typedef _Tp type; };
+template<class _Tp> using __type_identity_t = typename __type_identity<_Tp>::type;
+
----------------
jloser wrote:
> Quuxplusone wrote:
> > Moot now, but FYI, we already have this under the name `__identity_t`.
> Good to know, thanks. I didn't look around too hard since it was mostly a stab to try to get windows-dll builds working. I'll remove this anyway soon as you know.
Interesting. I had searched for pre c++20 version of `type_identity` during review, but didn't find an alternative.


================
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");
----------------
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...


================
Comment at: libcxx/test/std/strings/string.view/string.view.cons/from_range.pass.cpp:127
+
+int main(int, char**) {
+  test();
----------------
jloser wrote:
> jloser wrote:
> > Mordante wrote:
> > > The paper has a Thows clause `Throws:Any exception thrown byranges::data(r)andranges::size(r).`
> > > Can you add some tests to verify that behaviour?
> > Should be doable - good idea. I'll look into that once I figure out the duplicate symbol issue for windows-dll for `std::string_view::operator!=`. My attempted fix using `__type_identity_t` doesn't work.
> Added some tests for both cases! Clang doesn't like `throws` in a `constexpr` member function like `size()` or `data()`, so they're just a normal runtime test.
Yeah there's no `constexpr` exception handing (yet?), see http://eel.is/c++draft/expr.const#5.26


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113161