Details
- Reviewers
Mordante - Group Reviewers
Restricted Project - Commits
- rG6ed40418914b: [libc++] Implement ostringstream members of P0408R7 (Efficient Access to…
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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 |
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. Or do we need to have "requires" every time the standard says "Constraints:" ? |
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. |
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. |
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.
@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
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.