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

[Amirreza Ashouri via Phabricator via libcxx-commits]

AMP999 added inline comments.


================
Comment at: libcxx/include/sstream:402-409
+        string_type __result = [&]() {
+            const basic_string_view<_CharT, _Traits> __view = view();
+            if (__view.empty()) {
+                return string_type(__str_.get_allocator());
+            }
             auto __pos = __view.data() - __str_.data();
+            return string_type(std::move(__str_), __pos, __view.size(), __str_.get_allocator());
----------------
pfusik wrote:
> 
Your change looks OK to me.


================
Comment at: libcxx/test/std/input.output/string.streams/istringstream/istringstream.members/str.allocator_propagation.pass.cpp:13
+// inner string object to the new string returned from .str().
+// `str() const&` is specified to preserve the allocator (not copy the string).
+// `str() &&` isn't specified, but should preserve the allocator (move the string).
----------------
pfusik wrote:
> What do you mean by "not copy the string" ? It returns a string by value.
Yes, but the string it returns is not created by copying the stringbuf's associated string. The copy constructor would use `allocator_traits::select_on_container_copy_construction`.


================
Comment at: libcxx/test/std/input.output/string.streams/istringstream/istringstream.members/str.allocator_propagation.pass.cpp:14
+// `str() const&` is specified to preserve the allocator (not copy the string).
+// `str() &&` isn't specified, but should preserve the allocator (move the string).
+
----------------
pfusik wrote:
> Why "should" ?
Because that's consistent both with `.str() const&` (which preserves the allocator) and with the ordinary move-constructor of `string` (which preserves the allocator). I don't think the user would ever expect `.str() &&` to do anything but preserve the allocator.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157776