This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Implement C++20 P0408R7 (Efficient Access to basic_stringbuf's Buffer)
AbandonedPublic

Authored by pfusik on Jun 1 2023, 8:57 AM.

Details

Reviewers
None
Group Reviewers
Restricted Project

Diff Detail

Event Timeline

pfusik created this revision.Jun 1 2023, 8:57 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 1 2023, 8:57 AM
pfusik requested review of this revision.Jun 1 2023, 8:57 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 1 2023, 8:57 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
pfusik updated this revision to Diff 527449.Jun 1 2023, 9:08 AM

Rebased.

CI complains about missing stringbuf::str() const.
It's stringbuf::str() const & now, so it can be overloaded with stringbuf::str() &&.
How to handle this?

philnik added a subscriber: philnik.Jun 8 2023, 4:37 PM
philnik added inline comments.
libcxx/include/sstream
374–375

Why do you add a conditional noexcept without any guards here?

383–384

Otherwise this is super confusing IMO. The extra bits should fix your problem.

pfusik updated this revision to Diff 529855.Jun 9 2023, 1:44 AM

Added noexcept guard.
Hide the new str() overload from ABI.

pfusik marked 2 inline comments as done.Jun 9 2023, 1:47 AM
pfusik added inline comments.
libcxx/include/sstream
374–375

Fixed.

383–384

This doesn't look right: the & should be in C++20. What's _LIBCPP_BUILDING_LIBRARY ?

It looks similar to std::string::substr which doesn't use _LIBCPP_BUILDING_LIBRARY.

philnik added inline comments.Jun 9 2023, 8:44 AM
libcxx/include/sstream
383–384

_LIBCPP_BUILDING_LIBRARY is defined when building the static or shared library. It's not required for string::substr, because that's not part of the library. stringbuf::str() OTOH is part of the library through an extern template.

pfusik updated this revision to Diff 529985.Jun 9 2023, 9:10 AM
pfusik marked 2 inline comments as done.

Keep stringbuf::str() when building the library.
Avoid the _NOEXCEPT_ macro.

pfusik marked an inline comment as done.Jun 9 2023, 9:12 AM
pfusik updated this revision to Diff 530171.Jun 10 2023, 12:08 AM

Fix LIBCXX_ENABLE_DEBUG_MODE=ON failure by using string_view::data instead of string_view::begin.

Thanks for working on this. I mainly glossed over the patch.

libcxx/docs/Status/Cxx20Papers.csv
102

Please mention this in ReleaseNotes.rst too

libcxx/include/sstream
36

Pleas align // C++20 in the same column

285

I really dislike this macro. It's quite unclear what it does. Signatures like string str() && are valid. Looking at the number of use cases I rather ifdef at these places.

342

We use std in new code. We don't change existing code to avoid merge conflicts with open patches.

libcxx/test/std/input.output/string.streams/ostringstream/ostringstream.members/str.alloc.pass.cpp
18

I miss tests for const member functions.

pfusik updated this revision to Diff 530216.Jun 10 2023, 8:52 AM
pfusik marked 5 inline comments as done.

Address code review comments.
Include pre-C++20 str() in synopsis.

pfusik updated this revision to Diff 530217.Jun 10 2023, 8:55 AM

One const too far.

libcxx/test/std/input.output/string.streams/ostringstream/ostringstream.members/str.alloc.pass.cpp
18

Added const in alloc.pass.cpp tests, members and constructors.

pfusik updated this revision to Diff 530218.Jun 10 2023, 9:03 AM

Add const to one more test.

pfusik updated this revision to Diff 530642.Jun 12 2023, 12:44 PM

Ensure postconditions of str() &&

pfusik updated this revision to Diff 531798.Jun 15 2023, 9:43 AM

Ensure buffer is empty after str() &&.

Could you split the patch in 4 parts. I'm not too familiar with this code and it's not the easiest to grok so reviewing takes quite a bit of time. I mainly looked at streambuf, I still need to verify that there are tests for all post conditions.

Having smaller patches makes it a lot easier to find time to look at it.

libcxx/include/sstream
38
54
void swap(basic_stringbuf& rhs) noexcept(see below ); 
                                ~~~~~~~~~~~~~~~~~~~~

This misses an addition

60

please move the basic_string_view<char_type, traits_type> view() const noexcept; // C++20 here so it matches the synopsis.

Same for the str() && function.

359

This constraint http://eel.is/c++draft/stringbuf#cons-8 Constraints: is_same_v<SAlloc, Allocator> is false. is missing.

365

I would prefer this in its own ifdef block so the code follows the order of the wording in the Standard.
Normally I don't care too much, but this class has way to many constructors with small differences; so it's easy to look at "the wrong one".

456

move from __rhs.__str_.

466

Use after move of __rhs.__str_.

Note this was existing, but is a bug.

libcxx/test/std/input.output/string.streams/stringbuf/stringbuf.cons/alloc.pass.cpp
17–29

Typically we do one test per function overload. Can you split this in multiple tests?

The same for other tests.

pfusik updated this revision to Diff 532986.Jun 20 2023, 10:34 AM
pfusik marked 7 inline comments as done.

Addressing the review comments.
Will split to smaller changes.

libcxx/include/sstream
466

What fix would you suggest?

pfusik abandoned this revision.Jun 21 2023, 1:36 AM

Splitting to smaller chunks. First one: https://reviews.llvm.org/D153405