Details
- Reviewers
- None
- Group Reviewers
Restricted Project
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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?
| libcxx/include/sstream | ||
|---|---|---|
| 386–387 | _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. | |
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 | |
| 290 | 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. | |
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. | |
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–55 | 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. | |
| 462 | move from __rhs.__str_. | |
| 472 | 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. | |
Addressing the review comments.
Will split to smaller changes.
| libcxx/include/sstream | ||
|---|---|---|
| 472 | What fix would you suggest? | |
Please mention this in ReleaseNotes.rst too