This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Implement ostringstream members of P0408R7 (Efficient Access to basic_stringbuf's Buffer)
ClosedPublic

Authored by pfusik on Jul 14 2023, 2:11 AM.

Diff Detail

Event Timeline

pfusik created this revision.Jul 14 2023, 2:11 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 14 2023, 2:11 AM
pfusik requested review of this revision.Jul 14 2023, 2:11 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 14 2023, 2:11 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript

In general LGTM, modulo one nit. I like to have a quick look before the final approval.

libcxx/include/sstream
967

This overload should be constrained is_same_v<SAlloc,Allocator> is false

pfusik added inline comments.Jul 15 2023, 8:14 AM
libcxx/include/sstream
967

Also in istringstream and stringstream.

It seems to work fine without the explicit constraint, selecting the non-template constructor, e.g. ostringstream.cons/string.pass.cpp.
I think this is because https://timsong-cpp.github.io/cppwp/over.match.best#general-2.4

Or do we need to have "requires" every time the standard says "Constraints:" ?

Mordante added inline comments.Jul 15 2023, 12:06 PM
libcxx/include/sstream
967

The overload resolution indeed selects the expected function. However the function is a viable function, which it should not be per http://eel.is/c++draft/description#structure.specifications-3.1

Note I think that page of the standard contains a lot of information regarding the standard library. You might want to read a bit more on that page.

pfusik updated this revision to Diff 540730.Jul 15 2023, 12:35 PM
pfusik marked 2 inline comments as done.

Added a constructor constraint, also in istringstream.

Mordante accepted this revision.Jul 16 2023, 6:05 AM

LGTM after removing the unneeded constraint.

libcxx/include/sstream
846

Interestingly this one does not seem to be constrained. http://eel.is/c++draft/string.streams#istringstream.cons-6

The same in the paper. I can't find a reason in the WG21 review either. I assume it's a bug in the Standard. For now let's remove this one and maybe we should file an LWG issue to remove them from the constructors when not needed.

This revision is now accepted and ready to land.Jul 16 2023, 6:05 AM
pfusik updated this revision to Diff 540800.Jul 16 2023, 6:19 AM

Removed the istringstream constructor constraint.

This revision was landed with ongoing or failed builds.Jul 16 2023, 6:25 AM
This revision was automatically updated to reflect the committed changes.
pfusik marked an inline comment as done.

It looks like this change causes an out of range exception to be thrown for this snippet:

// ./bin/clang++ -std=c++20 /tmp/test.cc -g
#include <iostream>
#include <sstream>

int main() {
  std::ostringstream oss;
  std::cerr << std::move(oss).str();
  return 0;
}

This fails with libc++abi: terminating due to uncaught exception of type std::out_of_range: basic_string but doesn't prior to this patch. It looks like the failure comes from this bit here:

    _LIBCPP_HIDE_FROM_ABI_SSTREAM string_type str() && {
        const basic_string_view<_CharT, _Traits> __view = view();
        string_type __result(std::move(__str_), __view.data() - __str_.data(), __view.size());
        __str_.clear();
        __init_buf_ptrs();
        return __result;
    }

Where __view.data() - __str_.data() is an absurdly large value for the pos argument in the string ctor:

#11 0x00005555555a960d in std::__2::basic_string<char, std::__2::char_traits<char>, std::__2::allocator<char> >::basic_string (this=0x7fffffffd5f8, __str=..., __pos=18446603336221206952, __n=0, __a=...)

Perhaps the pos is actually a negative number or view.data() is actually nullptr? Would you be able to send out a fix or revert? Thanks.

pfusik added a comment.Aug 1 2023, 3:23 AM

@leonardchan Thank you for your report and analysis!
__view.data() was nullptr. I did not realize that the position must be valid even when the count is zero.
Fix: https://reviews.llvm.org/D156783