Moved to https://github.com/llvm/llvm-project/pull/66643

In D157602#4592023, @hans wrote:

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

Can you post the one you tried as starting point? I'll try to tweak it then.

What differs in the real world deployments vs what's tested in the libcxx CI; what test config would be needed to catch this in the CI?

Rebased. Added testing with test_allocator.

In D157602#4576735, @thakis wrote:

(and D155185 if it isn't yet) to the LLVM 17 branch too.

Fixes link failures reported by @thakis at https://reviews.llvm.org/D153709

In D156783#4550729, @Mordante wrote:

After you land the patch, can you put it up for backporting to LLVM-17?
(The instructions are here https://www.llvm.org/docs/GitHub.html#backporting-fixes-to-the-release-branches)

Rebased.

Refactor a function argument to a variable (NFC).

Rebased.

Fixes a bug reported by @leonardchan at https://reviews.llvm.org/D155276
In the bug scenario, __view.data() == nullptr. The offset is validated by the string constructor/assign even if count == 0.

@leonardchan Thank you for your report and analysis!
__view.data() was nullptr. I did not realize that the position must be valid even when the count is zero.
Fix: https://reviews.llvm.org/D156783

@ldionne Can we please continue this review before the 17.0 branch is created?

Rebased. Updated the status docs. A whitespace fix.

Removed the istringstream constructor constraint.

Added a constructor constraint.

Added a constructor constraint, also in istringstream.

This is just a small test cleanup: the directory and header comment said that it tests stringstream but it used istringstream.
The test was originally created this way: https://github.com/llvm/llvm-project/commit/1347d334518a3642243e97c7d173452443047e2f#diff-ea0557985f856925f1b019fcd64ee6d1ff0465ad8cd19a42b355e5b9c97589fe

Together with https://reviews.llvm.org/D155276 this concludes the paper implementation.
I didn't update the docs yet as that would conflict with https://reviews.llvm.org/D155276
(and I don't feel confident to use chained patches at this point)

Update the commit message with arc diff --edit.

Fix TODO pattern.
Add full issue link in the commit message.

This is symmetric to https://reviews.llvm.org/D154454

I think it's better as-is.

Rebased without conflicts.

Rebased.

I think https://reviews.llvm.org/D155185 addresses it better: there are two overloads to handle for now, there will be eight.

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

@Mordante You pointed this as a problem in https://reviews.llvm.org/D151896#inline-1482124 Could you please explain? Now I'm confused myself.

Mark move assignment/constructor tests as unsupported in C++03.

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

s/assert/LIBCPP_ASSERT/

In D154499#4474816, @philnik wrote:

I am quite certain that basic_stringbuf has to be in a valid but unspecified state

@philnik Are the tests for libc++ or the C++ standard? The former does specify the state.

Test source pointers set to null.

@ldionne std::basic_string<CharT, std::traits<CharT>, test_allocator<CharT>> is a different type than std::basic_string<CharT>.
I double-checked with printfs which constructor was called.

This fixes use-after-moves spotted by @Mordante in https://reviews.llvm.org/D151896#inline-1482124
The pointers are now set to null, same as in the default constructor of stringbuf.

I noticed a test I added in https://reviews.llvm.org/D153709 wasn't testing the constructor overload it was supposed to test. Instead, it tested the same overload as string-alloc.mode.pass.cpp.

Address review comments.

https://reviews.llvm.org/D153709 - only 11 overloads/tests.

Ok, I'll submit a patch implementing the whole paper in stringbuf. Since you asked for a separate test per overload, be prepared for two dozen tests. :)

Please land this change as I have no commit access. Thank you!

In D153405#4446388, @Mordante wrote:

Sorry, but would it be possible to to a patch per class instead, starting with stringbuf? That way I can keep track that all classes are done. The subtile differences in all overloads make it really annoying the review for correctness.

Reword comments.
More tests, descriptive classes.

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

Addressing the review comments.
Will split to smaller changes.

Ensure buffer is empty after str() &&.