[libcxx-commits] [PATCH] D153709: [libc++] Implement stringbuf members of P0408R7 (Efficient Access to basic_stringbuf's Buffer)

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

Thanks a lot for splitting, that really helped me to finally do a full review of this patch.
It looks mostely good, some minor things.



================
Comment at: libcxx/include/sstream:364
+#if _LIBCPP_STD_VER >= 20
+    template <class _SAlloc>
+    _LIBCPP_HIDE_FROM_ABI void str(const basic_string<char_type, traits_type, _SAlloc>& __s) {
----------------
http://eel.is/c++draft/stringbuf#members-15 `Constraints: is_same_v<SAlloc,Allocator> is false.`


================
Comment at: libcxx/include/sstream:422
     __hm_ = __hm == -1 ? nullptr : __p + __hm;
     __p = const_cast<char_type*>(__rhs.__str_.data());
     __rhs.setg(__p, __p, __p);
----------------
We still need to look at this issue.


================
Comment at: libcxx/include/sstream:469
     __mode_ = __rhs.__mode_;
     __p = const_cast<char_type*>(__rhs.__str_.data());
     __rhs.setg(__p, __p, __p);
----------------
Same here.


================
Comment at: libcxx/test/std/input.output/string.streams/stringbuf/stringbuf.cons/alloc.pass.cpp:13
+
+// template <class charT, class traits = char_traits<charT>, class Allocator = allocator<charT> >
+// class basic_stringbuf
----------------
Here and in the other tests.


================
Comment at: libcxx/test/std/input.output/string.streams/stringbuf/stringbuf.cons/string-alloc.mode.pass.cpp:34
+  const std::basic_stringbuf<CharT> buf(s, std::ios_base::in);
+  assert(buf.view() == SV("testing"));
+}
----------------
How about testing the allocator of `buf.str()`?
Maybe we should do even more testing the expected allocator is used. The `test_allocator` has ways to log that information.


================
Comment at: libcxx/test/std/input.output/string.streams/stringbuf/stringbuf.members/str.alloc.pass.cpp:31
+static void test() {
+  {
+    const std::basic_stringbuf<CharT> buf(STR("testing"));
----------------
This nesting is not needed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153709