[PATCH] D93912: [libc++][P1679] add string contains

[Zoe Carver via Phabricator via llvm-commits]

zoecarver added a comment.

A few small comments. This is looking really good :)



================
Comment at: libcxx/include/string:1450
+    constexpr _LIBCPP_INLINE_VISIBILITY
+    bool contains(const value_type* __s) const
+    { return __self_view(data(), size()).contains(__s); }
----------------
Thoughts on making this one noexcept as well? (I know it's not noexcept in the standard, does anyone know the reason for that?)


================
Comment at: libcxx/test/std/strings/basic.string/string.contains/contains.char.pass.cpp:35
+
+    return true;
+}
----------------
See my comment below... that means we don't need this either. 


================
Comment at: libcxx/test/std/strings/basic.string/string.contains/contains.char.pass.cpp:42
+    // FIXME: wait for constexpr std::string P0980
+    // static_assert(test());
+
----------------
I think we should leave this out. Whoever implements that can add tests as part of their patch. 


================
Comment at: libcxx/test/std/strings/basic.string/string.contains/contains.ptr.pass.cpp:22
+    {
+    typedef std::string S;
+    const char *s = "abcde";
----------------
Quuxplusone wrote:
> Could use `using S = std::string;` here, since this is a non-03 test.
We can also use `using` in C++03 now, just FYI. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93912