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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

I had a closer look at the patch this time and compared it with the papers. Some new remarks, but it's getting close.



================
Comment at: libcxx/docs/Status/Cxx20Papers.csv:102
 "`P0325R4 <https://wg21.link/P0325R4>`__","LWG","to_array from LFTS with updates","Cologne","|Complete|","10.0"
-"`P0408R7 <https://wg21.link/P0408R7>`__","LWG","Efficient Access to basic_stringbuf's Buffer","Cologne","",""
+"`P0408R7 <https://wg21.link/P0408R7>`__","LWG","Efficient Access to basic_stringbuf's Buffer","Cologne","|In Progress|",""
 "`P0466R5 <https://wg21.link/P0466R5>`__","LWG","Layout-compatibility and Pointer-interconvertibility Traits","Cologne","",""
----------------
The note needs to linked to this entry. See remark below.


================
Comment at: libcxx/docs/Status/Cxx20Papers.csv:195
 "","","","","","",""
 "`P2231R1 <https://wg21.link/P2231R1>`__","LWG","Missing constexpr in std::optional and std::variant","June 2021","|Partial| [#note-P2231]_","13.0"
 "`P2325R3 <https://wg21.link/P2325R3>`__","LWG","Views should not be required to be default constructible","June 2021","|Complete|","16.0","|ranges|"
----------------
Here is an example how to link the note.


================
Comment at: libcxx/include/sstream:512
+{
+    if (__mode_ & ios_base::out)
+    {
----------------
It seems the tests only validate this code path. The `basic_stringbuf` is always uses the default `which` argument. I assume the same issue applies to the original tests you copied. It would be great to improve these tests too in a separate review.


================
Comment at: libcxx/include/sstream:512
+{
+    if (__mode_ & ios_base::out)
+    {
----------------
Mordante wrote:
> It seems the tests only validate this code path. The `basic_stringbuf` is always uses the default `which` argument. I assume the same issue applies to the original tests you copied. It would be great to improve these tests too in a separate review.
The same applies for the `_Traits` argument, it's only tested for the default traits. We don't have specific `char_traits` in our test frame-work but you can probably do something like

```
template <class CharT>
struct my_char_traits : public char_traits<CharT> {};
```
and use these traits.

Nowadays we try to have more extensive tests then we used to have in the past so that's why the `str()` tests are as they are.




================
Comment at: libcxx/include/sstream:492
+basic_string_view<_CharT, _Traits>
+basic_stringbuf<_CharT, _Traits, _Allocator>::view() const noexcept
+{
----------------
pfusik wrote:
> Mordante wrote:
> > pfusik wrote:
> > > Mordante wrote:
> > > > Please format the new code with clang-format so it matches our "normal" style. 
> > > How do I do that?
> > You can use git clang-format like `git clang-format-16 HEAD^` this formats the changes in the last commit.
>     fox at wiesmac llvm-project % git clang-format HEAD^
>     clang-format did not modify any files
> 
Odd it doesn't work for me either, normally it does. When I select this hunk manully and format it I get
```
template <class _CharT, class _Traits, class _Allocator>
_LIBCPP_HIDE_FROM_ABI basic_string_view<_CharT, _Traits>
basic_stringbuf<_CharT, _Traits, _Allocator>::view() const noexcept {
    if (__mode_ & ios_base::out) {
        if (__hm_ < this->pptr())
            __hm_ = this->pptr();
        return basic_string_view<_CharT, _Traits>(this->pbase(), __hm_);
    } else if (__mode_ & ios_base::in)
        return basic_string_view<_CharT, _Traits>(this->eback(), this->egptr());
    return basic_string_view<_CharT, _Traits>();
}

```


================
Comment at: libcxx/test/std/input.output/string.streams/istringstream/istringstream.members/view.pass.cpp:24
+static std::string_view get_view(const std::istringstream& ss) noexcept { return ss.view(); }
+
+int main(int, char**) {
----------------
pfusik wrote:
> Mordante wrote:
> > Using something along these lines avoid some code duplication. This also makes it easier to adapt the tests if we can use `basic_stringstream` with other character types like `char8_t`.
> Now I support your original claim that tests should be simple. There's not much duplication here.
This is something we do in a lot of tests, so I like to keep that pattern.


================
Comment at: libcxx/test/std/input.output/string.streams/ostringstream/ostringstream.members/view.pass.cpp:36
+    assert(ss.view() == "abc9");
+    const std::ostringstream css("abc");
+    static_assert(noexcept(css.view()));
----------------
Please also test the `const view()` returns the proper value. This test would pass with a `const` member that returns `basic_string_view<charT>{}`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148641