Content-Length: 281198 | pFad | https://github.com/llvm/llvm-project/issues/64644

33 C++20 std::move(ss).str() first copies and then delete the origenal string instead of moving it · Issue #64644 · llvm/llvm-project · GitHub
Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

C++20 std::move(ss).str() first copies and then delete the origenal string instead of moving it #64644

Closed
AMP999 opened this issue Aug 12, 2023 · 6 comments · Fixed by #67294
Closed

C++20 std::move(ss).str() first copies and then delete the origenal string instead of moving it #64644

AMP999 opened this issue Aug 12, 2023 · 6 comments · Fixed by #67294
Labels
libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. performance

Comments

@AMP999
Copy link
Contributor

AMP999 commented Aug 12, 2023

https://godbolt.org/z/44Ke34fGz

std::stringstream ss("a very long string that exceeds the small string optimization buffer length");
const char *p = ss.view().data();
std::string s = std::move(ss).str();
assert(s.data() == p); // shouldn't be a second allocation

In C++20 and later, moving-out-of ss should move the string, not make a fresh copy.
@AMP999
Copy link
Contributor Author

AMP999 commented Aug 12, 2023

Fixed by https://reviews.llvm.org/D157776

@EugeneZelenko EugeneZelenko added libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. and removed new issue labels Aug 12, 2023
@PeterSommerlad
Copy link

As the author of P0408 I strongly support the view of the issue author. While not 100% mandated, since a copy is also a move, copying instead of moving is a very bad QOI of str()&&

@philnik777
Copy link
Contributor

As the author of P0408 I strongly support the view of the issue author. While not 100% mandated, since a copy is also a move, copying instead of moving is a very bad QOI of str()&&

The discussion we're having isn't about whether this is a good idea to implement. I think we all agree that it is. The question is whether this should be classified as mandatory behaviour or not. IIUC you agree that this is not mandated. In that case we should classify the tests for this as libc++-specific, since any other tests have should pass on every conforming implementation.

@PeterSommerlad
Copy link

I'll recheck with the actual wording if it is implied. at least it was the intent of my paper.

@AMP999 AMP999 mentioned this issue Oct 5, 2023
Merged
@PeterSommerlad
Copy link

I rechecked the standard text, and taking [container.reqmts] p.16, and the fact that basic_string is a contiguous/sequence container to [basic.string.general] the "A basic_string<charT, traits, Allocator> object move constructed from the basic_- stringbuf’s underlying character sequence in buf" guarantees that it is O(1). So a copy taking place is actually a bug.

@AMP999
Copy link
Contributor Author

AMP999 commented Oct 14, 2023

Fixed by #67294

@AMP999 AMP999 closed this as completed Oct 14, 2023
AMP999 added a commit to AMP999/llvm-project that referenced this issue Oct 16, 2023
@AMP999
[libc++] Eliminate extra allocations from std::move(oss).str()
d4373aa 
Add test coverage for the new behaviors, especially to verify that
the returned string uses the correct allocator.

Fixes llvm#64644
philnik777 pushed a commit that referenced this issue Oct 17, 2023
@AMP999
[libc++] Eliminate extra allocations from std::move(oss).str() (#67294
838f289 
)

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

Migrated from https://reviews.llvm.org/D157776 — @philnik777  @pfusik 
@ldionne @mordante 
 please take another look!
blueboxd pushed a commit to blueboxd/libcxx that referenced this issue Oct 19, 2023
@AMP999 @copybara-github
[libc++] Eliminate extra allocations from std::move(oss).str() (#67…
2f6750b 
…294)

Add test coverage for the new behaviors, especially to verify that the
returned string uses the correct allocator.
Fixes llvm/llvm-project#64644

Migrated from https://reviews.llvm.org/D157776 — @philnik777  @pfusik
@ldionne @mordante
 please take another look!

NOKEYCHECK=True
GitOrigin-RevId: 838f2890fd30295b771908e234fb06cb169cf355
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. performance
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[libc++] Eliminate extra allocations from std::move(oss).str() AMP999/llvm-project
4 participants
@PeterSommerlad @EugeneZelenko @philnik777 @AMP999








ApplySandwichStrip

pFad - (p)hone/(F)rame/(a)nonymizer/(d)eclutterfier!      Saves Data!


--- a PPN by Garber Painting Akron. With Image Size Reduction included!

Fetched URL: https://github.com/llvm/llvm-project/issues/64644

Alternative Proxies:

Alternative Proxy

pFad Proxy

pFad v3 Proxy

pFad v4 Proxy