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

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const requested changes to this revision.
var-const added inline comments.
This revision now requires changes to proceed.


================
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, removed in C++23
+    basic_string substr(size_type pos = 0, size_type n = npos) const&;                          // since C++23
----------------
Ultranit: `s/since/in/`? Since it's removed in the very next release, it doesn't really make sense to say it's constexpr _since_ C++20.


================
Comment at: libcxx/include/string:880
+    _LIBCPP_HIDE_FROM_ABI constexpr
+    basic_string(basic_string&& __str, size_type __pos, size_type __n, const _Allocator& __alloc = _Allocator())
+        : __r_(__default_init_tag(), __alloc) {
----------------
Other constructors also insert the newly-created string into the debug database, should this be done here as well?


================
Comment at: libcxx/include/string:882
+        : __r_(__default_init_tag(), __alloc) {
+      if (__pos > __str.size())
+        __throw_out_of_range();
----------------
Can you please add some blank lines so that this function is split into logical blocks visually? I'd suggest one after the error check in the beginning (to separate argument validation from the main flow), one before the `else` branch, and possibly one before `_Traits::move`.


================
Comment at: libcxx/include/string:885
+      auto __len = std::min(__n, __str.size() - __pos);
+      if (__alloc_traits::is_always_equal::value || __alloc == __str.__alloc()) {
+        __r_.first() = __str.__r_.first();
----------------
Is this check necessary? Wouldn't `__alloc == __str.__alloc()` return `true` in that case?


================
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
----------------
Mordante wrote:
> 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?
Using a single line is consistent with other entries in the synopsis and makes it obvious to which function the comment refers to (whereas for the two-line version, it could refer to either the preceding or the following line).


================
Comment at: libcxx/include/string:879
+    _LIBCPP_HIDE_FROM_ABI constexpr
+    basic_string(basic_string&& __str, size_type __pos, size_type __n, const _Allocator& __alloc = _Allocator())
+        : __r_(__default_init_tag(), __alloc) {
----------------
avogelsgesang wrote:
> philnik wrote:
> > avogelsgesang wrote:
> > > So far, all constructors and functions were defined outside the interface definition. Was there a conscious decision to no longer do so? If not, I think we should keep implementations out of the class definitions for consistency.
> > That's not true. See `basic_string(__uininitialized_size_tag, size_type, const allocator_type&)` for example. I don't actually know why the function definitions were written out of line. It just leads to massive amounts of boiler-plate.
> ok, your call
I think we've been trying to move away from the old style of defining these out-of-class. In addition to boilerplate, it makes modifying the interface, SFINAE in particular, a bigger pain than it should be.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131668