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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

Mainly glossed over the patch, I didn't do a real review.



================
Comment at: libcxx/include/sstream:403
         const basic_string_view<_CharT, _Traits> __view = view();
-        if (!__view.empty()) {
-            auto __pos = __view.data() - __str_.data();
-            __result.assign(std::move(__str_), __pos, __view.size());
-        }
+        auto __pos = __view.empty() ? 0 : __view.data() - __str_.data();
+        string_type __result(std::move(__str_), __pos, __view.size(), __str_.get_allocator());
----------------
`auto` seems wrong here; there's no guarantee `same_as<int, ptrdiff_t>` is true.


================
Comment at: libcxx/include/string:975
+  // Extension: Support these overloads in C++20 rather than C++23.
+  // Otherwise C++20's `basic_stringbuf::str() &&` is hard to implement.
   _LIBCPP_HIDE_FROM_ABI constexpr
----------------
Extensions should be documented in our user documentation.

Can you please elaborate why it's hard? That makes it easier to understand the motivation for the reader (and the reviewer).


================
Comment at: libcxx/test/std/input.output/string.streams/istringstream/istringstream.members/str.allocator_propagation.pass.cpp:61
+    // [stringbuf.members]/6 specifies that the allocator is copied,
+    // not SOCCC'ed.
+    //
----------------
Please write our what COCCC'ed is. 


================
Comment at: libcxx/test/std/input.output/string.streams/istringstream/istringstream.members/str.allocator_propagation.pass.cpp:96
+  using SS = std::basic_istringstream<CharT, std::char_traits<CharT>, std::pmr::polymorphic_allocator<CharT>>;
+  using S = std::pmr::basic_string<CharT>;
+  alignas(void*) char buf[80 * sizeof(CharT)];
----------------
pmr is not available on Apple backdepoyment targets. Best add `// UNSUPPORTED: availability-pmr-missing` to these tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157776