This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Eliminate extra allocations from `std::move(oss).str()`
AbandonedPublic

Authored by ldionne on Aug 12 2023, 5:01 AM.

Details

Reviewers
pfusik
philnik
AMP999
Group Reviewers
Restricted Project
Summary

As an extension, support string(std::move(s), pos, n) in C++20
as well as C++23. Otherwise ,it's difficult to implement the
behavior for std::move(oss).str() in C++

Add test coverage for the new behaviors, especially to verify
the returned string uses the correct allocator.

Fixes the issue https://github.com/llvm/llvm-project/issues/64644

Diff Detail

Event Timeline

AMP999 created this revision.Aug 12 2023, 5:01 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 12 2023, 5:01 AM
AMP999 requested review of this revision.Aug 12 2023, 5:01 AM
Herald added a reviewer: Restricted Project. · View Herald TranscriptAug 12 2023, 5:01 AM
pfusik accepted this revision.Aug 12 2023, 7:40 AM

LGTM, modulo a lambda that I think could be simplified and some unclear test comments.

libcxx/include/sstream
402–408
408

I think we don't need this __str_.clear() anymore.

libcxx/test/std/input.output/string.streams/istringstream/istringstream.members/str.allocator_propagation.pass.cpp
14

What do you mean by "not copy the string" ? It returns a string by value.

15

Why "should" ?

philnik requested changes to this revision.Aug 12 2023, 8:41 AM
philnik added a subscriber: philnik.

Given that there is no requirement in the standard for this, all the tests should be libc++ specific. No implementation is required to have this behaviour.

libcxx/include/sstream
408

We do if the allocator is not equal after copying it. Then the old string isn't cleared.

libcxx/include/string
973–976

Let's not add this as an extension in C++20. It's easy enough to add a private constructor for this.

This revision now requires changes to proceed.Aug 12 2023, 8:41 AM
AMP999 added inline comments.Aug 12 2023, 9:44 AM
libcxx/include/sstream
402–408

Your change looks OK to me.

libcxx/test/std/input.output/string.streams/istringstream/istringstream.members/str.allocator_propagation.pass.cpp
14

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.

15

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.

AMP999 updated this revision to Diff 549707.Aug 13 2023, 8:59 AM

Add @pfusik 's change.

AMP999 marked an inline comment as done.Aug 13 2023, 9:00 AM
AMP999 added inline comments.Aug 13 2023, 11:29 AM
libcxx/include/sstream
408

Copies are equal by definition. But if the old string fits in the small-string buffer, the old string won't be cleared, either.

libcxx/include/string
973–976

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?

AMP999 updated this revision to Diff 549740.Aug 13 2023, 12:18 PM

I appreciate it if someone helps me with the test failures. I'm not sure why the "Apple back-deployment" targets are all failing.

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

libcxx/include/sstream
403–407

auto seems wrong here; there's no guarantee same_as<int, ptrdiff_t> is true.

libcxx/include/string
975

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).

libcxx/test/std/input.output/string.streams/istringstream/istringstream.members/str.allocator_propagation.pass.cpp
62

Please write our what COCCC'ed is.

97

pmr is not available on Apple backdepoyment targets. Best add // UNSUPPORTED: availability-pmr-missing to these tests.

AMP999 updated this revision to Diff 551534.Aug 18 2023, 8:53 AM

Addressed review comments, removed string(string&&, pos, n, alloc) constructor from C++20 mode again.

AMP999 marked 4 inline comments as done.Aug 18 2023, 9:07 AM
AMP999 marked 5 inline comments as done.Aug 18 2023, 5:05 PM
frederick-vs-ja added inline comments.
libcxx/include/string
973–976

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 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 here).

I don't know what is libstdc++ planning to do. Its rvalue str() overload looks simple enough, so I guess it won't get P2438R2 implemented in C++20 mode just for convenience.

@philnik gentle ping!

Re "there is no requirement in the standard," I think the use of the term "move constructed" in https://eel.is/c++draft/stringbuf#members-10.sentence-1 is that requirement. The original paper P0408 also contains this test case https://github.com/PeterSommerlad/SC22WG21_Papers/blob/master/workspace/Test_basic_stringbuf_efficient/src/Testp0407-p0408-efficientStringstreams.cpp#L105-L112 which won't succeed unless we transfer ownership here. Transfer-of-ownership is clearly P0408's intent.

Re supporting the new basic_string constructor in C++20 mode: Okay, I have removed it from this patch.

@philnik gentle ping!

Re "there is no requirement in the standard," I think the use of the term "move constructed" in https://eel.is/c++draft/stringbuf#members-10.sentence-1 is that requirement. The original paper P0408 also contains this test case https://github.com/PeterSommerlad/SC22WG21_Papers/blob/master/workspace/Test_basic_stringbuf_efficient/src/Testp0407-p0408-efficientStringstreams.cpp#L105-L112 which won't succeed unless we transfer ownership here. Transfer-of-ownership is clearly P0408's intent.

A moved-from string has an unspecified state, so there is no way to check what actually happened.

libcxx/include/sstream
408

Right. In that case the clear() is indeed redundant. The old string will be cleared, but even if that wasn't the case, there is no requirement that the string is cleared.

libcxx/include/string
973–975

The extension is still here.

1335

This is a way more canonical name.

pfusik added inline comments.Aug 28 2023, 3:13 PM
libcxx/include/sstream
408

There is one:

basic_string<charT, traits, Allocator> str() &&;
Postconditions: The underlying character sequence buf is empty (...)

AMP999 updated this revision to Diff 556332.Sep 8 2023, 7:25 PM

The extension is removed.

ldionne commandeered this revision.Sep 25 2023, 7:10 AM
ldionne added a reviewer: AMP999.
ldionne added a subscriber: ldionne.

Abandoning to clean up the review queue.

ldionne abandoned this revision.Sep 25 2023, 7:10 AM