[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:410
+        }();
         __str_.clear();
         __init_buf_ptrs();
----------------
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.


================
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.
----------------
philnik wrote:
> Let's not add this as an extension in C++20. It's easy enough to add a private constructor for this.
Do we have a preferred style for adding "private constructors"? For example, could I change this overload set to:
```
#if _LIBCPP_STD_VER >= 23
  _LIBCPP_HIDE_FROM_ABI constexpr
  basic_string(basic_string&& __str, size_type __pos, const _Allocator& __alloc = _Allocator())
      : basic_string(__libcpp_extension_t(), std::move(__str), __pos, npos, __alloc) {}
  _LIBCPP_HIDE_FROM_ABI constexpr
  basic_string(basic_string&& __str, size_type __pos, size_type __n, const _Allocator& __alloc = _Allocator())
      : basic_string(__libcpp_extension_t(), std::move(__str), __pos, __n, __alloc) {}
#endif

  _LIBCPP_HIDE_FROM_ABI constexpr
  basic_string(__libcpp_extension_t, basic_string&& __str, size_type __pos, size_type __n, const _Allocator& __alloc = _Allocator())
      : __r_(__default_init_tag(), __alloc) {
[...]
```
But I'm also interested in hearing others' opinions. It seems to me that if std::string(std::move(s), 0) copies in C++20 and moves in C++23, anyone actually relying on that fact won't be able to upgrade safely from C++20 to C++23. (Anyone not relying on that fact won't care... except for performance, where move is better.) I think we should make this overload do the same thing in all versions where it exists (i.e. C++20 and later). What are libstdc++ and MSVC planning to do?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157776