This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Implement istringstream members of P0408R7 (Efficient Access to basic_stringbuf's Buffer)
ClosedPublic

Authored by pfusik on Jul 4 2023, 10:05 AM.

Diff Detail

Event Timeline

pfusik created this revision.Jul 4 2023, 10:05 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 4 2023, 10:05 AM
pfusik requested review of this revision.Jul 4 2023, 10:05 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 4 2023, 10:05 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript

Thanks! This look quite good, a few small issues.

libcxx/include/sstream
813

I prefer the wording of the standard, I know the existing code doesn't do that but let's do it for the new code.

863

This function is not constrained in the Standard. Why is it needed?

libcxx/test/std/input.output/string.streams/istringstream/istringstream.cons/mode.alloc.pass.cpp
27

I would avoid std::ios_base::in in these tests if possible. This should be automatically be set.

libcxx/test/std/input.output/string.streams/istringstream/istringstream.members/str.move.pass.cpp
32

Is the moved from state specified?

pfusik added inline comments.Jul 4 2023, 11:26 AM
libcxx/include/sstream
863

If we omit this constraint, we get:

call to member function 'str' is ambiguous
        ss.str(" 789");

I think the standard should include it.

libcxx/test/std/input.output/string.streams/istringstream/istringstream.cons/mode.alloc.pass.cpp
27

Why? What to use instead of std::ios_base::in ?

pfusik marked an inline comment as done.Jul 4 2023, 11:46 AM
pfusik added inline comments.
libcxx/test/std/input.output/string.streams/istringstream/istringstream.members/str.move.pass.cpp
32

It is:

29.8.3.4 Member functions [istringstream.members]
basic_string<charT,traits,Allocator> str() &&;
Effects: Equivalent to: return std::move(*rdbuf()).str();

29.8.2.4 Member functions [stringbuf.members]
basic_string<charT, traits, Allocator> str() &&;
Postconditions: The underlying character sequence buf is empty

Mordante added inline comments.Jul 10 2023, 10:18 AM
libcxx/include/sstream
813

This item is not done.

863

Thanks. I want to look at this locally. If this indeed is needed it looks like a defect in the Standard.

When this is a bug in the Standard we should file an LWG issue and mention that in the code.
(No need to take an action now, I first want to test.)

libcxx/test/std/input.output/string.streams/istringstream/istringstream.cons/mode.alloc.pass.cpp
27

When we don't set it we can test whether the in flag is set. I would set the binary flag.

libcxx/test/std/input.output/string.streams/istringstream/istringstream.members/str.move.pass.cpp
32

Thanks!

pfusik updated this revision to Diff 538743.Jul 10 2023, 11:10 AM
pfusik marked 5 inline comments as done.

s/&/std::addressof/
s/std:ios_base::in/std::ios_base::binary/

libcxx/include/sstream
813

Sorry, I'm still learning Phabricator. It's a little confusing that both "arc diff" and "Submit" on the web submit the "Done" flags.

Mordante added inline comments.Jul 10 2023, 11:22 AM
libcxx/include/sstream
863

This is correct. I got reminded the Equivalent to wording "inherits" the constrains.
(And indeed tests fails without the constrain.)

Mordante accepted this revision.Jul 10 2023, 11:25 AM

LGTM!

libcxx/include/sstream
813

I didn't know arc diff did that. It doesn't for me.

This revision is now accepted and ready to land.Jul 10 2023, 11:25 AM
pfusik marked 3 inline comments as done.Jul 12 2023, 2:28 PM

Please land it so I can continue with ostringstream and stringstream.

Please land it so I can continue with ostringstream and stringstream.

Can you rebase the patch to make sure the CI is green. Note in Phabricator you can created stacked patches, which allows you to continue working on the other patches while they are reviewed.

pfusik updated this revision to Diff 540071.Jul 13 2023, 9:22 AM

Rebased.

Rebased without conflicts.

I also need to apply the https://reviews.llvm.org/D155185 workaround to istringstream.
If https://reviews.llvm.org/D154454 lands first, I can edit https://reviews.llvm.org/D155185
If https://reviews.llvm.org/D155185 lands first, I can edit https://reviews.llvm.org/D154454

Nice to see you have commit access now!