[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 accepted this revision.
Mordante added a comment.
This revision is now accepted and ready to land.

Thanks LGTM! I'll land the patch on your behalf.



================
Comment at: libcxx/include/sstream:365
+    template <class _SAlloc>
+      requires (!std::is_same_v<_SAlloc, allocator_type>)
+    _LIBCPP_HIDE_FROM_ABI void str(const basic_string<char_type, traits_type, _SAlloc>& __s) {
----------------
Nit: we only use `std::` for function calls to avoid ADL.

Note since the rest of the patch is fine, I'll fix this before landing.


================
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);
----------------
pfusik wrote:
> Mordante wrote:
> > We still need to look at this issue.
> I think it is okay to use `nullptr` instead of `__p` obtained from the moved string.
> But since it is an unrelated fix, I'd prefer to do this in a separate change.
+1 for doing it in a separate patch.


================
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"));
+}
----------------
pfusik wrote:
> Mordante wrote:
> > 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.
> How?
> `stringbuf` fixes its allocator at construction time. Here we are testing a constructor overload that doesn't accept an allocator.
Good point.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153709