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

[Piotr Fusik via Phabricator via libcxx-commits]

pfusik accepted this revision.
pfusik added a comment.

LGTM, modulo a lambda that I think could be simplified and some unclear test 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());
----------------



================
Comment at: libcxx/include/sstream:410
+        }();
         __str_.clear();
         __init_buf_ptrs();
----------------
I think we don't need this `__str_.clear()` anymore.


================
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).
----------------
What do you mean by "not copy the string" ? It returns a string by value.


================
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).
+
----------------
Why "should" ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157776