[libcxx-commits] [PATCH] D151896: [libc++] Implement C++20 P0408R7 (Efficient Access to basic_stringbuf's Buffer)

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

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.



================
Comment at: libcxx/include/sstream:38
+    basic_stringbuf(ios_base::openmode which, const allocator_type& a);                // C++20
+    explicit basic_stringbuf(const basic_string<char_type, traits_type, allocator_type>&& s,
+                             ios_base::openmode which = ios_base::in | ios_base::out); // C++20
----------------



================
Comment at: libcxx/include/sstream:54
     basic_stringbuf& operator=(basic_stringbuf&& rhs);
     void swap(basic_stringbuf& rhs);
 
----------------
```
void swap(basic_stringbuf& rhs) noexcept(see below ); 
                                ~~~~~~~~~~~~~~~~~~~~
```
This misses an addition 


================
Comment at: libcxx/include/sstream:59
+    basic_string<char_type, traits_type, allocator_type> str() const;                  // before C++20
+    basic_string<char_type, traits_type, allocator_type> str() const &;                // C++20
     void str(const basic_string<char_type, traits_type, allocator_type>& s);
----------------
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.


================
Comment at: libcxx/include/sstream:358
+    template <class _SAlloc>
+    _LIBCPP_HIDE_FROM_ABI explicit basic_stringbuf(const basic_string<char_type, traits_type, _SAlloc>& __s,
+                                                   ios_base::openmode __wch = ios_base::in | ios_base::out)
----------------
This constraint http://eel.is/c++draft/stringbuf#cons-8 `Constraints: is_same_v<SAlloc, Allocator> is false.` is missing.


================
Comment at: libcxx/include/sstream:364
+
+    _LIBCPP_HIDE_FROM_ABI basic_stringbuf(basic_stringbuf&& __rhs, const allocator_type& __a)
+        : basic_stringbuf(__rhs.__mode_, __a) {
----------------
I would prefer this in its own ifdef block so the code follows the order of the wording in the Standard.
Normally I don't care too much, but this class has way to many constructors with small differences; so it's easy to look at "the wrong one".


================
Comment at: libcxx/include/sstream:458
     ptrdiff_t __hm = __rhs.__hm_ == nullptr ? -1 : __rhs.__hm_ - __p;
     __str_ = _VSTD::move(__rhs.__str_);
     __p = const_cast<char_type*>(__str_.data());
----------------
move from `__rhs.__str_`.


================
Comment at: libcxx/include/sstream:468
     __hm_ = __hm == -1 ? nullptr : __p + __hm;
     __p = const_cast<char_type*>(__rhs.__str_.data());
     __rhs.setg(__p, __p, __p);
----------------
Use after move of `__rhs.__str_`.

Note this was existing, but is a bug.


================
Comment at: libcxx/test/std/input.output/string.streams/stringbuf/stringbuf.cons/alloc.pass.cpp:16-28
+// explicit basic_stringbuf(const Allocator& a)
+//     : basic_stringbuf(ios_base::in | ios_base::out, a) {}
+// basic_stringbuf(ios_base::openmode which, const Allocator& a);
+// template <class SAlloc>
+// basic_stringbuf(const basic_string<charT, traits, SAlloc>& s, const Allocator& a)
+//     : basic_stringbuf(s, ios_base::in | ios_base::out, a) {}
+// template <class SAlloc>
----------------
Typically we do one test per function overload. Can you split this in multiple tests?

The same for other tests.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151896/new/

https://reviews.llvm.org/D151896