[libcxx-commits] [PATCH] D148641: [libc++] Add C++20 stringstream::view()

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

Thanks for working on this! I mainly looked at the high-level parts not compared against the paper yet.

Does this patch implement the entire paper or not? If not please make sure you add a note on the status page to explain which parts are and are not done. (We might remove the synopsis in the future.)



================
Comment at: libcxx/include/sstream:37
 
     // [stringbuf.assign] Assign and swap:
     basic_stringbuf& operator=(basic_stringbuf&& rhs);
----------------
Please update `libcxx/docs/Status/Cxx20.rst` with the implementation status of P0408R7. It would be great to put that number+paper name in the commit message too. That makes it easier to find the associated paper.


================
Comment at: libcxx/include/sstream:449-457
-    if (__mode_ & ios_base::out)
-    {
-        if (__hm_ < this->pptr())
-            __hm_ = this->pptr();
-        return string_type(this->pbase(), __hm_, __str_.get_allocator());
-    }
-    else if (__mode_ & ios_base::in)
----------------
The feature should be guarded so it is only available in C++20 and later.

The same applies to where you add the `view` member.


================
Comment at: libcxx/include/sstream:492
+basic_string_view<_CharT, _Traits>
+basic_stringbuf<_CharT, _Traits, _Allocator>::view() const noexcept
+{
----------------
Please format the new code with clang-format so it matches our "normal" style. 


================
Comment at: libcxx/test/std/input.output/string.streams/istringstream/istringstream.members/str.pass.cpp:14
 
 // void str(const basic_string<charT,traits,Allocator>& s);
 
----------------
Please create a new test for the view (can be a copy past of this file) and update the synopsis here.

That test will need an `// UNSUPPORTED: c++03, c++11, c++14, c++17` line since it is C++20 only.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148641