- Group Reviewers
_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.
Thanks for working on this. I mainly glossed over the patch.
Please mention this in ReleaseNotes.rst too
Pleas align // C++20 in the same column
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.
We use std in new code. We don't change existing code to avoid merge conflicts with open patches.
I miss tests for const member functions.
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.
void swap(basic_stringbuf& rhs) noexcept(see below ); ~~~~~~~~~~~~~~~~~~~~
This misses an addition
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.
This constraint http://eel.is/c++draft/stringbuf#cons-8 Constraints: is_same_v<SAlloc, Allocator> is false. is missing.
I would prefer this in its own ifdef block so the code follows the order of the wording in the Standard.
move from __rhs.__str_.
Use after move of __rhs.__str_.
Note this was existing, but is a bug.
Typically we do one test per function overload. Can you split this in multiple tests?
The same for other tests.