[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:874
     _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_AFTER_CXX17
-    basic_string(const basic_string& __str, size_type __pos,
-                 const _Allocator& __a = _Allocator());
+    basic_string(const basic_string& __str, size_type __pos, const _Allocator& __a = _Allocator());
+
----------------
I'm a little surprised this constructor is not conditionally `noexcept`. Do you know if this is deliberate or an omission in the paper?


================
Comment at: libcxx/include/string:888
+      auto __len = std::min(__n, __str.size() - __pos);
+      if (__alloc_traits::is_always_equal::value || __alloc == __str.__alloc()) {
+        __r_.first() = __str.__r_.first();
----------------
Can you confirm that the `is_always_equal` check optimizes away even without using `if constexpr`?


================
Comment at: libcxx/include/string:889
+      if (__alloc_traits::is_always_equal::value || __alloc == __str.__alloc()) {
+        __r_.first() = __str.__r_.first();
+        __str.__default_init();
----------------
Hmm, the move constructor instead does this:
```
        if (__libcpp_is_constant_evaluated()) {
            __zero();
            __r_.first().__l = __str.__r_.first().__l;
        } else {
            __r_.first().__r = __str.__r_.first().__r;
        }
```
Your version is simpler. Is there a functional difference? If these are equivalent, I think we should simplify the move constructor as well (not in this patch). If not equivalent, is there a reason why the new constructor can use a simple assignment without the conditional logic?


================
Comment at: libcxx/include/string:892
+
+        _Traits::move(data(), data() + __pos, __len);
+        __set_size(__len);
----------------
Consider a comment like `Move the given string then shrink it in place`.


================
Comment at: libcxx/include/string:892
+
+        _Traits::move(data(), data() + __pos, __len);
+        __set_size(__len);
----------------
var-const wrote:
> Consider a comment like `Move the given string then shrink it in place`.
Reevaluating `data()` calls `to_address()` and `is_long()` repeatedly. Would it make sense to store the result in an intermediate variable?


================
Comment at: libcxx/include/string:896
+      } else {
+        __init(__str.data() + __pos, __len);
+      }
----------------
Consider adding a comment like `Perform a copy because allocators have different types`.


================
Comment at: libcxx/include/string:899
+
+      std::__debug_db_insert_c(this);
+    }
----------------
The move constructor also performs
```
    if (__is_long())
        std::__debug_db_swap(this, &__str);
```
I presume this is necessary here as well -- @ldionne would know more.


================
Comment at: libcxx/include/string:1303
+    _LIBCPP_HIDE_FROM_ABI constexpr
+    basic_string substr(size_type __pos = 0, size_type __n = npos) const& {
+      return basic_string(*this, __pos, __n, __alloc());
----------------
+1 to @Mordante's comment re. ABI stability. This should also be documented in the commit message (either why the ABI break doesn't happen in our case, or what we are doing to mitigate it).


================
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) {
----------------
philnik wrote:
> var-const wrote:
> > Other constructors also insert the newly-created string into the debug database, should this be done here as well?
> Yes, I think so. I couldn't find any comprehensive tests for the debug mode though. Do you know if we have any/where they are?
I suspect we don't have those tests, but let's confirm with @ldionne.


================
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());
+    }
----------------
Mordante wrote:
> philnik wrote:
> > Mordante wrote:
> > > philnik wrote:
> > > > Mordante wrote:
> > > > > This seems to be https://wg21.link/lwg3752, can you mention in the status page we implement this not yet accepted LWG-issue?
> > > > I wouldn't consider LWG3752 to be implemented. It doesn't propose a solution and IMO the right thing would be to use `select_on_container_copy_construction` on the `const&` overload and passing the allocator on the `&&` overload.
> > > But this deviates from the wording in the paper, there it's defined as
> > > `return basic_string(*this, pos, n);` which uses `basic_string`'s default allocator.
> > > So there should be some comments as why we have this non-conforming behaviour.
> > I don't know why we have the non-conforming behaviour. I just didn't want to change the behaviour in this patch. I've filed https://llvm.org/PR57190.
> I still would like a comment in the code mentioning this is non-standard behaviour and a link to the bug report.
> That way when somebody sees this and wonders why it's clear this was done intentionally.
Is the proposed resolution of LWG3752 to pass the allocator? I couldn't find the discussion it refers to, but because it says "we will fix it", I take it to mean that the current behavior, i.e., passing `__alloc()` should be changed, and the paper merely asks for confirmation.



================
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
----------------
var-const wrote:
> 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).
> 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).

Sorry, looks like I misread the original comment -- please disregard.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131668