[libcxx-commits] [PATCH] D131668: [libc++] Implement P2438R2 (std::string::substr() &&)

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

Thanks for working on this. Some comments and I see the entire MacOS build fails. Is that a glitch or this patch?



================
Comment at: libcxx/docs/ReleaseNotes.rst:41
 - P2499R0 - ``string_view`` range constructor should be ``explicit``
+- P2438R2 - ``std::string::substr() &&``
 
----------------
I miss an update to the status page.


================
Comment at: libcxx/docs/ReleaseNotes.rst:41
 - P2499R0 - ``string_view`` range constructor should be ``explicit``
+- P2438R2 - ``std::string::substr() &&``
 
----------------
Mordante wrote:
> I miss an update to the status page.
Interestingly there's no feature-test macro in the paper.


================
Comment at: libcxx/include/string:888
+        __str.__default_init();
+        _Traits::move(data(), data() + __pos, __len);
+        __set_size(__len);
----------------
Did you measure the overhead for this move when `__pos == 0`?


================
Comment at: libcxx/include/string:1297
+    basic_string substr(size_type __pos = 0, size_type __n = npos) const& {
+      return basic_string(*this, __pos, __n, __alloc());
+    }
----------------
This seems to be https://wg21.link/lwg3752, can you mention in the status page we implement this not yet accepted LWG-issue?


================
Comment at: libcxx/include/string:261
     size_type copy(value_type* s, size_type n, size_type pos = 0) const;                        // constexpr since C++20
-    basic_string substr(size_type pos = 0, size_type n = npos) const;                           // constexpr since C++20
-
+    basic_string substr(size_type pos = 0, size_type n = npos) const;                           // constexpr since C++20, const & since C++23
+    constexpr basic_string substr(size_type pos = 0, size_type n = npos) &&; // since C++23
----------------
avogelsgesang wrote:
> I would rather split this into two lines;
> 
>     basic_string substr(size_type pos = 0, size_type n = npos) const;                           // constexpr since C++20, removed in C++23
>     basic_string substr(size_type pos = 0, size_type n = npos) const&;                           // since C++23
Can you improve the formatting and alignment here and above in the synopsis?


================
Comment at: libcxx/test/std/strings/basic.string/string.cons/substr_rvalue.pass.cpp:163
+  test_string<std::basic_string<char, std::char_traits<char>, min_allocator<char>>>(min_allocator<char>{});
+  test_string<std::basic_string<char, std::char_traits<char>, test_allocator<char>>>(test_allocator<char>{42});
+
----------------
For new tests I really would like to see all character types being tested.


================
Comment at: libcxx/test/std/strings/basic.string/string.cons/substr_rvalue.pass.cpp:163
+  test_string<std::basic_string<char, std::char_traits<char>, min_allocator<char>>>(min_allocator<char>{});
+  test_string<std::basic_string<char, std::char_traits<char>, test_allocator<char>>>(test_allocator<char>{42});
+
----------------
Mordante wrote:
> For new tests I really would like to see all character types being tested.
How about using different `char_traits` too?


================
Comment at: libcxx/test/std/strings/basic.string/string.ops/string_substr/substr_rvalue.pass.cpp:13
+
+// constexpr basic_string substr(size_type pos = 0, size_type n = npos) &&;
+
----------------
Please update the synopsis in the `constexpr basic_string substr(size_type pos = 0, size_type n = npos) const;` test.


================
Comment at: libcxx/test/std/strings/basic.string/string.ops/string_substr/substr_rvalue.pass.cpp:65
+  test_string<std::basic_string<char, std::char_traits<char>, min_allocator<char>>>();
+  test_string<std::basic_string<char, std::char_traits<char>, test_allocator<char>>>();
+
----------------
For new tests I really would like to see all character types being tested.


================
Comment at: libcxx/test/std/strings/basic.string/string.ops/string_substr/substr_rvalue.pass.cpp:65
+  test_string<std::basic_string<char, std::char_traits<char>, min_allocator<char>>>();
+  test_string<std::basic_string<char, std::char_traits<char>, test_allocator<char>>>();
+
----------------
Mordante wrote:
> For new tests I really would like to see all character types being tested.
How about using different `char_traits` too?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131668