[libcxx-commits] [PATCH] D157776: [libc++] Eliminate extra allocations from `std::move(oss).str()`

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added a comment.

In D157776#4622524 <https://reviews.llvm.org/D157776#4622524>, @AMP999 wrote:

> @philnik gentle ping!
>
> Re "there is no requirement in the standard," I think the use of the term "move constructed" in https://eel.is/c++draft/stringbuf#members-10.sentence-1 is that requirement. The original paper P0408 also contains this test case https://github.com/PeterSommerlad/SC22WG21_Papers/blob/master/workspace/Test_basic_stringbuf_efficient/src/Testp0407-p0408-efficientStringstreams.cpp#L105-L112 which won't succeed unless we transfer ownership here. Transfer-of-ownership is clearly P0408's intent.

A moved-from string has an unspecified state, so there is no way to check what actually happened.



================
Comment at: libcxx/include/sstream:410
+        }();
         __str_.clear();
         __init_buf_ptrs();
----------------
AMP999 wrote:
> philnik wrote:
> > pfusik wrote:
> > > I think we don't need this `__str_.clear()` anymore.
> > We do if the allocator is not equal after copying it. Then the old string isn't cleared.
> Copies are equal by definition. But if the old string fits in the small-string buffer, the old string won't be cleared, either.
Right. In that case the `clear()` is indeed redundant. The old string will be cleared, but even if that wasn't the case, there is no requirement that the string is cleared.


================
Comment at: libcxx/include/string:973-975
+#if _LIBCPP_STD_VER >= 20
+  // Extension: Support these overloads in C++20 rather than C++23.
+  // Otherwise C++20's `basic_stringbuf::str() &&` is hard to implement.
----------------
The extension is still here.


================
Comment at: libcxx/include/string:1335
+  _LIBCPP_HIDE_FROM_ABI constexpr
+  void __pilfering_assign(basic_string&& __str, size_type __pos, size_type __len) {
+    // Pilfer the allocation from __str.
----------------
This is a way more canonical name.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157776