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

[A. Jiang via Phabricator via libcxx-commits]

frederick-vs-ja added inline comments.


================
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.
----------------
AMP999 wrote:
> 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?
> anyone actually relying on that fact won't be able to upgrade safely from C++20 to C++23

It's extremely weird to write `std::move` while relying that it doesn't work.

> What are libstdc++ and MSVC planning to do?

I've [[ https://github.com/microsoft/STL/pull/3057 | implemented P2438R2 in MSVC STL ]] in C++23 mode only. MSVC STL's `basic_stringbuf` can't directly reuse `basic_string` at this moment due to ABI compatibility (see [[ https://github.com/microsoft/STL/issues/938 | here ]]).

I don't know what is libstdc++ planning to do. [[ https://github.com/gcc-mirror/gcc/blob/c28c579f0dd9cd27f90df9aff7cbdb2db1c23b3b/libstdc%2B%2B-v3/include/std/sstream#L248-L257 | Its rvalue str() overload ]] looks simple enough, so I guess it won't get P2438R2 implemented in C++20 mode just for convenience.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157776