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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante requested changes to this revision.
Mordante added a comment.
This revision now requires changes to proceed.

In general looks good, but the tests need some improvements.



================
Comment at: libcxx/test/std/strings/string.view/string.view.cons/deduct.pass.cpp:10
+// UNSUPPORTED: libcpp-no-concepts
+// XFAIL: apple-clang-12.0
+
----------------
Minor nit, XFAIL isn't required, since `// UNSUPPORTED: libcpp-no-concepts` achieves the same.


================
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>);
+  }
----------------
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`


================
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>);
+  }
----------------
Why do you want to test with `int`?


================
Comment at: libcxx/test/std/strings/string.view/string.view.cons/deduct.pass.cpp:34
+  }
+}
+
----------------
There should be a `return 0;`. On some platforms this is even required for `main`.


================
Comment at: libcxx/test/std/strings/string.view/string.view.cons/from_iterator_iterator.pass.cpp:20
+
+constexpr bool test() {
+  const char val[] = "test";
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110718