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

[Joe Loser via Phabricator via libcxx-commits]

jloser marked 3 inline comments as done.
jloser added inline comments.


================
Comment at: libcxx/test/std/strings/string.view/string.view.cons/deduct.pass.cpp:10
+// UNSUPPORTED: libcpp-no-concepts
+// XFAIL: apple-clang-12.0
+
----------------
Mordante wrote:
> Minor nit, XFAIL isn't required, since `// UNSUPPORTED: libcpp-no-concepts` achieves the same.
Are you sure? Without the `XFAIL`, I was seeing CI failures only for `AppleClang 12.0.0` as it couldn't resolve `contiguous_iterator` from what I could tell. The build failure link is https://buildkite.com/llvm-project/libcxx-ci/builds/5619.

BTW, we're about to drop support for `AppleClang 12.0.0` as I talked with @ldionne the other day. `AppleClang 13.0.0` just came out on 9/20 IIRC which has full support for concepts and then we can remove this `XFAIL`.


================
Comment at: libcxx/test/std/strings/string.view/string.view.cons/deduct.pass.cpp:26
+    std::basic_string_view sv{a.cbegin(), a.cend()};
+    ASSERT_SAME_TYPE(decltype(sv), std::basic_string_view<char>);
+  }
----------------
Mordante wrote:
> Since this test only tests the types, it could be a `compile.pass.cpp`. But I see the other deduction tests are a `pass.cpp`. They also test whether the result is the expected result. Can you also add the other character types in this test? For example have a look at `libcxx/test/std/strings/basic.string/string.cons/string_view_deduction.pass.cpp`
Added  other character types and also test the result now. Good call!


================
Comment at: libcxx/test/std/strings/string.view/string.view.cons/deduct.pass.cpp:32
+    std::basic_string_view sv{a.cbegin(), a.cend()};
+    ASSERT_SAME_TYPE(decltype(sv), std::basic_string_view<int>);
+  }
----------------
Mordante wrote:
> Why do you want to test with `int`?
Just as another character type. I decided to stick with `char`, `wchar_t`, and friends instead now.


================
Comment at: libcxx/test/std/strings/string.view/string.view.cons/from_iterator_iterator.pass.cpp:20
+
+constexpr bool test() {
+  const char val[] = "test";
----------------
Mordante wrote:
> Here it would be nice to add a test for all character types. This can be done using templates. You can use `MAKE_CSTRING` from `libcxx/test/support/make_string.h` to create the `val` based on a template argument.
Added a test for a variety of other character types. I added a `MAKE_STRING_VIEW` macro so our test function can be used in a constexpr context still (since we don't have fully `constexpr` `string` yet). This allows me to have `static_assert(test()))` work still.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110718