Details
- Reviewers
- None
- Group Reviewers
Restricted Project
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
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 | ||
---|---|---|
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. |
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. |
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 | 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. | |
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. |
Addressing the review comments.
Will split to smaller changes.
libcxx/include/sstream | ||
---|---|---|
466 | What fix would you suggest? |
Please mention this in ReleaseNotes.rst too