[libcxx-commits] [PATCH] D93912: [libc++][P1679] add string contains

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

Quuxplusone requested changes to this revision.
Quuxplusone added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/include/string:330
+    bool contains(const charT* s) const;                               // C++23
+
     bool __invariants() const;
----------------
Before libc++ starts adding C++23 stuff, I think it needs someone (notme) to do a "C++20 exists now" audit. References to "2a" should be changed to "20" — especially in the test suite, where I keep tripping up by typing `--param std=c++20` instead of `--param std=c++2a` — and then a "2b" mode needs to be added.

I weakly recommend using "2b" instead of "23" here, for pedantic accuracy. I admit that it is //very likely// that C++2b will be C++23, and that by recommending "2b" I'm just making work for somebody three years from now. :P


================
Comment at: libcxx/include/string:1437
     bool ends_with(const value_type* __s) const _NOEXCEPT
     { return ends_with(__self_view(__s)); }
 #endif
----------------
Incidentally, I wonder why this existing code isn't `return __self_view(data(), size()).ends_with(__s)`. I like the symmetrical way you did `contains` below, and wish `starts_with` and `ends_with` would match it... unless there's a good reason they were done this way originally.


================
Comment at: libcxx/test/std/strings/basic.string/string.contains/contains.ptr.pass.cpp:22
+    {
+    typedef std::string S;
+    const char *s = "abcde";
----------------
Could use `using S = std::string;` here, since this is a non-03 test.


================
Comment at: libcxx/test/std/strings/basic.string/string.contains/contains.ptr.pass.cpp:30
+//  S   s4  { s + 1, 4 };
+//  S   s5  { s,     5 };
+    S  sNot { "xyz", 3 };
----------------
Why commented out?


================
Comment at: libcxx/test/std/strings/string.view/string.view.template/contains.char.pass.cpp:37
+
+#if TEST_STD_VER > 11
+    {
----------------
This is always true; you don't need the `#if`.
You don't need `constexpr_char_traits` in '20 and later (which includes this test).

Also, please do
```
constexpr bool test() {
    std::string_view sv1;
    std::string_view sv2 = "abcde";
    ASSERT_NOEXCEPT(sv1.contains('e'));
    assert(!sv1.contains('c'));
    [...]
    return true;
}

int main(int, char **)
{
    test();
    static_assert(test());
}
```
to get constexpr coverage without repeating the code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93912