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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

Thanks! This look quite good, a few small issues.



================
Comment at: libcxx/include/sstream:813
+    _LIBCPP_HIDE_FROM_ABI basic_istringstream(ios_base::openmode __wch, const _Allocator& __a)
+        : basic_istream<_CharT, _Traits>(&__sb_), __sb_(__wch | ios_base::in, __a) {}
+
----------------
I prefer the wording of the standard, I know the existing code doesn't do that but let's do it for the new code.


================
Comment at: libcxx/include/sstream:863
+    template <class _SAlloc>
+      requires __is_allocator<_SAlloc>::value
+    _LIBCPP_HIDE_FROM_ABI basic_string<char_type, traits_type, _SAlloc> str(const _SAlloc& __sa) const {
----------------
This function is not constrained in the Standard. Why is it needed?


================
Comment at: libcxx/test/std/input.output/string.streams/istringstream/istringstream.cons/mode.alloc.pass.cpp:27
+  const test_allocator<CharT> a(2);
+  const std::basic_istringstream<CharT, std::char_traits<CharT>, test_allocator<CharT>> ss(std::ios_base::in, a);
+  assert(ss.rdbuf()->get_allocator() == a);
----------------
I would avoid `std::ios_base::in` in these tests if possible. This should be automatically be set.


================
Comment at: libcxx/test/std/input.output/string.streams/istringstream/istringstream.members/str.move.pass.cpp:32
+    assert(s == STR("testing"));
+    assert(ss.view().empty());
+  }
----------------
Is the moved from state specified?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154454