This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Add C++20 stringstream::view()
ClosedPublic

Authored by pfusik on Apr 18 2023, 9:41 AM.

Details

Reviewers
philnik
Mordante
Group Reviewers
Restricted Project
Commits
rG49007a020c14: [libc++] Add C++20 stringstream::view()

Diff Detail

Event Timeline

pfusik created this revision.Apr 18 2023, 9:41 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 18 2023, 9:41 AM
pfusik requested review of this revision.Apr 18 2023, 9:41 AM
philnik set the repository for this revision to rG LLVM Github Monorepo.Apr 18 2023, 9:50 AM
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald TranscriptApr 18 2023, 9:50 AM
philnik requested changes to this revision.Apr 18 2023, 9:54 AM
philnik added a subscriber: philnik.

Please make sure you did everything listed at https://libcxx.llvm.org/Contributing.html#pre-commit-check-list. Feel free to ask here or on Discord (in the #libcxx channel) if you have any questions.

libcxx/include/sstream
878

This should be _LIBCPP_HIDE_FROM_ABI instead.

This revision now requires changes to proceed.Apr 18 2023, 9:54 AM
pfusik updated this revision to Diff 514681.Apr 18 2023, 10:32 AM

s/_LIBCPP_INLINE_VISIBILITY/_LIBCPP_HIDE_FROM_ABI/
Added the test.

pfusik marked an inline comment as done.Apr 18 2023, 10:35 AM

Please make sure you did everything listed at https://libcxx.llvm.org/Contributing.html#pre-commit-check-list. Feel free to ask here or on Discord (in the #libcxx channel) if you have any questions.

I have an error in my setup: https://libcxx.llvm.org/BuildingLibcxx.html#cmake-ninja-mingw

CMake Error at C:/0/src/llvm-project/libcxx/cmake/Modules/HandleLibCXXABI.cmake:59 (add_library):
  No SOURCES given to target: libcxx-abi-headers-generate-private-headers
pfusik updated this revision to Diff 514687.Apr 18 2023, 10:38 AM

git clang-format

pfusik updated this revision to Diff 514827.Apr 18 2023, 11:16 PM

Move the tests to libcxx/test/std/input.output/string.streams

Could you please help me with my local setup?

C:\0\src\llvm-project>cmake -G Ninja -S runtimes -B buildlib -DCMAKE_C_COMPILER=clang -DCMAKE_CXX_COMPILER=clang++ -DLLVM_ENABLE_RUNTIMES=libcxx -DLIBCXX_CXX_ABI=libstdc++
-- The C compiler identification is Clang 16.0.1
-- The CXX compiler identification is Clang 16.0.1
-- The ASM compiler identification is Clang with GNU-like command-line
-- Found assembler: C:/msys64/mingw64/bin/clang.exe
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: C:/msys64/mingw64/bin/clang.exe - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: C:/msys64/mingw64/bin/clang++.exe - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Could NOT find LLVM (missing: LLVM_DIR)
-- Could NOT find Clang (missing: Clang_DIR)
-- Performing Test CXX_SUPPORTS_UNWINDLIB_EQ_NONE_FLAG
-- Performing Test CXX_SUPPORTS_UNWINDLIB_EQ_NONE_FLAG - Success
-- Performing Test C_SUPPORTS_START_NO_UNUSED_ARGUMENTS
-- Performing Test C_SUPPORTS_START_NO_UNUSED_ARGUMENTS - Success
-- Performing Test CXX_SUPPORTS_NOSTDLIBXX_FLAG
-- Performing Test CXX_SUPPORTS_NOSTDLIBXX_FLAG - Success
-- Performing Test CXX_SUPPORTS_NOSTDINCXX_FLAG
-- Performing Test CXX_SUPPORTS_NOSTDINCXX_FLAG - Success
-- Performing Test SUPPORTS_FVISIBILITY_INLINES_HIDDEN_FLAG
-- Performing Test SUPPORTS_FVISIBILITY_INLINES_HIDDEN_FLAG - Success
-- Performing Test C_SUPPORTS_WERROR_DATE_TIME
-- Performing Test C_SUPPORTS_WERROR_DATE_TIME - Success
-- Performing Test CXX_SUPPORTS_WERROR_DATE_TIME
-- Performing Test CXX_SUPPORTS_WERROR_DATE_TIME - Success
-- Performing Test C_SUPPORTS_WERROR_UNGUARDED_AVAILABILITY_NEW
-- Performing Test C_SUPPORTS_WERROR_UNGUARDED_AVAILABILITY_NEW - Success
-- Performing Test CXX_SUPPORTS_WERROR_UNGUARDED_AVAILABILITY_NEW
-- Performing Test CXX_SUPPORTS_WERROR_UNGUARDED_AVAILABILITY_NEW - Success
-- Performing Test CXX_SUPPORTS_MISSING_FIELD_INITIALIZERS_FLAG
-- Performing Test CXX_SUPPORTS_MISSING_FIELD_INITIALIZERS_FLAG - Success
-- Performing Test C_SUPPORTS_IMPLICIT_FALLTHROUGH_FLAG
-- Performing Test C_SUPPORTS_IMPLICIT_FALLTHROUGH_FLAG - Success
-- Performing Test CXX_SUPPORTS_IMPLICIT_FALLTHROUGH_FLAG
-- Performing Test CXX_SUPPORTS_IMPLICIT_FALLTHROUGH_FLAG - Success
-- Performing Test C_SUPPORTS_COVERED_SWITCH_DEFAULT_FLAG
-- Performing Test C_SUPPORTS_COVERED_SWITCH_DEFAULT_FLAG - Success
-- Performing Test CXX_SUPPORTS_COVERED_SWITCH_DEFAULT_FLAG
-- Performing Test CXX_SUPPORTS_COVERED_SWITCH_DEFAULT_FLAG - Success
-- Performing Test CXX_SUPPORTS_CLASS_MEMACCESS_FLAG
-- Performing Test CXX_SUPPORTS_CLASS_MEMACCESS_FLAG - Failed
-- Performing Test CXX_SUPPORTS_NOEXCEPT_TYPE_FLAG
-- Performing Test CXX_SUPPORTS_NOEXCEPT_TYPE_FLAG - Success
-- Performing Test CXX_WONT_WARN_ON_FINAL_NONVIRTUALDTOR
-- Performing Test CXX_WONT_WARN_ON_FINAL_NONVIRTUALDTOR - Success
-- Performing Test CXX_SUPPORTS_SUGGEST_OVERRIDE_FLAG
-- Performing Test CXX_SUPPORTS_SUGGEST_OVERRIDE_FLAG - Success
-- Performing Test CXX_WSUGGEST_OVERRIDE_ALLOWS_ONLY_FINAL
-- Performing Test CXX_WSUGGEST_OVERRIDE_ALLOWS_ONLY_FINAL - Success
-- Performing Test C_WCOMMENT_ALLOWS_LINE_WRAP
-- Performing Test C_WCOMMENT_ALLOWS_LINE_WRAP - Failed
-- Performing Test C_SUPPORTS_STRING_CONVERSION_FLAG
-- Performing Test C_SUPPORTS_STRING_CONVERSION_FLAG - Success
-- Performing Test CXX_SUPPORTS_STRING_CONVERSION_FLAG
-- Performing Test CXX_SUPPORTS_STRING_CONVERSION_FLAG - Success
-- Performing Test C_SUPPORTS_MISLEADING_INDENTATION_FLAG
-- Performing Test C_SUPPORTS_MISLEADING_INDENTATION_FLAG - Success
-- Performing Test CXX_SUPPORTS_MISLEADING_INDENTATION_FLAG
-- Performing Test CXX_SUPPORTS_MISLEADING_INDENTATION_FLAG - Success
-- Performing Test C_SUPPORTS_CTAD_MAYBE_UNSPPORTED_FLAG
-- Performing Test C_SUPPORTS_CTAD_MAYBE_UNSPPORTED_FLAG - Success
-- Performing Test CXX_SUPPORTS_CTAD_MAYBE_UNSPPORTED_FLAG
-- Performing Test CXX_SUPPORTS_CTAD_MAYBE_UNSPPORTED_FLAG - Success
-- Performing Test C_SUPPORTS_FNO_FUNCTION_SECTIONS
-- Performing Test C_SUPPORTS_FNO_FUNCTION_SECTIONS - Success
-- Performing Test C_SUPPORTS_FFUNCTION_SECTIONS
-- Performing Test C_SUPPORTS_FFUNCTION_SECTIONS - Success
-- Performing Test CXX_SUPPORTS_FFUNCTION_SECTIONS
-- Performing Test CXX_SUPPORTS_FFUNCTION_SECTIONS - Success
-- Performing Test C_SUPPORTS_FDATA_SECTIONS
-- Performing Test C_SUPPORTS_FDATA_SECTIONS - Success
-- Performing Test CXX_SUPPORTS_FDATA_SECTIONS
-- Performing Test CXX_SUPPORTS_FDATA_SECTIONS - Success
-- Looking for os_signpost_interval_begin
-- Looking for os_signpost_interval_begin - not found
-- Found Python3: C:/msys64/mingw64/bin/python3.10.exe (found version "3.10.11") found components: Interpreter
-- LLVM host triple: x86_64-w64-windows-gnu
-- LLVM default target triple: x86_64-w64-windows-gnu
-- Using libc++ testing configuration: C:/0/src/llvm-project/libcxx/test/configs/llvm-libc++-mingw.cfg.in
-- Looking for fopen in c
-- Looking for fopen in c - not found
-- Looking for __gcc_personality_v0 in gcc_s
-- Looking for __gcc_personality_v0 in gcc_s - not found
-- Performing Test C_SUPPORTS_COMMENT_LIB_PRAGMA
-- Performing Test C_SUPPORTS_COMMENT_LIB_PRAGMA - Failed
-- Looking for pthread_create in pthread
-- Looking for pthread_create in pthread - found
-- Looking for ccos in m
-- Looking for ccos in m - found
-- Looking for clock_gettime in rt
-- Looking for clock_gettime in rt - not found
-- Looking for __atomic_fetch_add_8 in atomic
-- Looking for __atomic_fetch_add_8 in atomic - found
CMake Warning at C:/0/src/llvm-project/libcxx/cmake/Modules/HandleLibCXXABI.cmake:54 (message):
  Failed to find cxxabi.h in
Call Stack (most recent call first):
  C:/0/src/llvm-project/libcxx/cmake/Modules/HandleLibCXXABI.cmake:87 (import_private_headers)
  C:/0/src/llvm-project/libcxx/CMakeLists.txt:493 (include)


CMake Warning at C:/0/src/llvm-project/libcxx/cmake/Modules/HandleLibCXXABI.cmake:54 (message):
  Failed to find bits/c++config.h in
Call Stack (most recent call first):
  C:/0/src/llvm-project/libcxx/cmake/Modules/HandleLibCXXABI.cmake:87 (import_private_headers)
  C:/0/src/llvm-project/libcxx/CMakeLists.txt:493 (include)


CMake Warning at C:/0/src/llvm-project/libcxx/cmake/Modules/HandleLibCXXABI.cmake:54 (message):
  Failed to find bits/os_defines.h in
Call Stack (most recent call first):
  C:/0/src/llvm-project/libcxx/cmake/Modules/HandleLibCXXABI.cmake:87 (import_private_headers)
  C:/0/src/llvm-project/libcxx/CMakeLists.txt:493 (include)


CMake Warning at C:/0/src/llvm-project/libcxx/cmake/Modules/HandleLibCXXABI.cmake:54 (message):
  Failed to find bits/cpu_defines.h in
Call Stack (most recent call first):
  C:/0/src/llvm-project/libcxx/cmake/Modules/HandleLibCXXABI.cmake:87 (import_private_headers)
  C:/0/src/llvm-project/libcxx/CMakeLists.txt:493 (include)


CMake Warning at C:/0/src/llvm-project/libcxx/cmake/Modules/HandleLibCXXABI.cmake:54 (message):
  Failed to find bits/cxxabi_tweaks.h in
Call Stack (most recent call first):
  C:/0/src/llvm-project/libcxx/cmake/Modules/HandleLibCXXABI.cmake:87 (import_private_headers)
  C:/0/src/llvm-project/libcxx/CMakeLists.txt:493 (include)


CMake Warning at C:/0/src/llvm-project/libcxx/cmake/Modules/HandleLibCXXABI.cmake:54 (message):
  Failed to find bits/cxxabi_forced.h in
Call Stack (most recent call first):
  C:/0/src/llvm-project/libcxx/cmake/Modules/HandleLibCXXABI.cmake:87 (import_private_headers)
  C:/0/src/llvm-project/libcxx/CMakeLists.txt:493 (include)


-- Performing Test CXX_SUPPORTS_FALIGNED_ALLOCATION_FLAG
-- Performing Test CXX_SUPPORTS_FALIGNED_ALLOCATION_FLAG - Success
-- Performing Test CXX_SUPPORTS_FVISIBILITY_INLINES_HIDDEN_FLAG
-- Performing Test CXX_SUPPORTS_FVISIBILITY_INLINES_HIDDEN_FLAG - Success
-- Performing Test CXX_SUPPORTS_FVISIBILITY_EQ_HIDDEN_FLAG
-- Performing Test CXX_SUPPORTS_FVISIBILITY_EQ_HIDDEN_FLAG - Success
-- Performing Test CXX_SUPPORTS_WALL_FLAG
-- Performing Test CXX_SUPPORTS_WALL_FLAG - Success
-- Performing Test CXX_SUPPORTS_WEXTRA_FLAG
-- Performing Test CXX_SUPPORTS_WEXTRA_FLAG - Success
-- Performing Test CXX_SUPPORTS_WNEWLINE_EOF_FLAG
-- Performing Test CXX_SUPPORTS_WNEWLINE_EOF_FLAG - Success
-- Performing Test CXX_SUPPORTS_WSHADOW_FLAG
-- Performing Test CXX_SUPPORTS_WSHADOW_FLAG - Success
-- Performing Test CXX_SUPPORTS_WWRITE_STRINGS_FLAG
-- Performing Test CXX_SUPPORTS_WWRITE_STRINGS_FLAG - Success
-- Performing Test CXX_SUPPORTS_WNO_UNUSED_PARAMETER_FLAG
-- Performing Test CXX_SUPPORTS_WNO_UNUSED_PARAMETER_FLAG - Success
-- Performing Test CXX_SUPPORTS_WNO_LONG_LONG_FLAG
-- Performing Test CXX_SUPPORTS_WNO_LONG_LONG_FLAG - Success
-- Performing Test CXX_SUPPORTS_WERROR_EQ_RETURN_TYPE_FLAG
-- Performing Test CXX_SUPPORTS_WERROR_EQ_RETURN_TYPE_FLAG - Success
-- Performing Test CXX_SUPPORTS_WEXTRA_SEMI_FLAG
-- Performing Test CXX_SUPPORTS_WEXTRA_SEMI_FLAG - Success
-- Performing Test CXX_SUPPORTS_WUNDEF_FLAG
-- Performing Test CXX_SUPPORTS_WUNDEF_FLAG - Success
-- Performing Test CXX_SUPPORTS_WUNUSED_TEMPLATE_FLAG
-- Performing Test CXX_SUPPORTS_WUNUSED_TEMPLATE_FLAG - Success
-- Performing Test CXX_SUPPORTS_WFORMAT_NONLITERAL_FLAG
-- Performing Test CXX_SUPPORTS_WFORMAT_NONLITERAL_FLAG - Success
-- Performing Test CXX_SUPPORTS_WNO_USER_DEFINED_LITERALS_FLAG
-- Performing Test CXX_SUPPORTS_WNO_USER_DEFINED_LITERALS_FLAG - Success
-- Performing Test CXX_SUPPORTS_WNO_COVERED_SWITCH_DEFAULT_FLAG
-- Performing Test CXX_SUPPORTS_WNO_COVERED_SWITCH_DEFAULT_FLAG - Success
-- Performing Test CXX_SUPPORTS_WNO_SUGGEST_OVERRIDE_FLAG
-- Performing Test CXX_SUPPORTS_WNO_SUGGEST_OVERRIDE_FLAG - Success
-- Performing Test CXX_SUPPORTS_WNO_ERROR_FLAG
-- Performing Test CXX_SUPPORTS_WNO_ERROR_FLAG - Success
-- Performing Test CXX_SUPPORTS_EHSC_FLAG
-- Performing Test CXX_SUPPORTS_EHSC_FLAG - Failed
-- Adding Benchmark: algorithms.partition_point.bench.cpp
-- Adding Benchmark: equal.bench.cpp
-- Adding Benchmark: lower_bound.bench.cpp
-- Adding Benchmark: make_heap.bench.cpp
-- Adding Benchmark: make_heap_then_sort_heap.bench.cpp
-- Adding Benchmark: min.bench.cpp
-- Adding Benchmark: min_max_element.bench.cpp
-- Adding Benchmark: pop_heap.bench.cpp
-- Adding Benchmark: push_heap.bench.cpp
-- Adding Benchmark: ranges_make_heap.bench.cpp
-- Adding Benchmark: ranges_make_heap_then_sort_heap.bench.cpp
-- Adding Benchmark: ranges_pop_heap.bench.cpp
-- Adding Benchmark: ranges_push_heap.bench.cpp
-- Adding Benchmark: ranges_sort.bench.cpp
-- Adding Benchmark: ranges_sort_heap.bench.cpp
-- Adding Benchmark: ranges_stable_sort.bench.cpp
-- Adding Benchmark: sort.bench.cpp
-- Adding Benchmark: sort_heap.bench.cpp
-- Adding Benchmark: stable_sort.bench.cpp
-- Adding Benchmark: dynamic_cast.bench.cpp
-- Adding Benchmark: allocation.bench.cpp
-- Adding Benchmark: deque.bench.cpp
-- Adding Benchmark: deque_iterator.bench.cpp
-- Adding Benchmark: filesystem.bench.cpp
-- Adding Benchmark: format_to_n.bench.cpp
-- Adding Benchmark: format_to.bench.cpp
-- Adding Benchmark: format.bench.cpp
-- Adding Benchmark: formatted_size.bench.cpp
-- Adding Benchmark: formatter_float.bench.cpp
-- Adding Benchmark: formatter_int.bench.cpp
-- Adding Benchmark: function.bench.cpp
-- Adding Benchmark: join_view.bench.cpp
-- Adding Benchmark: lexicographical_compare_three_way.bench.cpp
-- Adding Benchmark: map.bench.cpp
-- Adding Benchmark: monotonic_buffer.bench.cpp
-- Adding Benchmark: ordered_set.bench.cpp
-- Adding Benchmark: std_format_spec_string_unicode.bench.cpp
-- Adding Benchmark: string.bench.cpp
-- Adding Benchmark: stringstream.bench.cpp
-- Adding Benchmark: to_chars.bench.cpp
-- Adding Benchmark: unordered_set_operations.bench.cpp
-- Adding Benchmark: util_smartptr.bench.cpp
-- Adding Benchmark: variant_visit_1.bench.cpp
-- Adding Benchmark: variant_visit_2.bench.cpp
-- Adding Benchmark: variant_visit_3.bench.cpp
-- Adding Benchmark: vector_operations.bench.cpp
-- ABI list file not generated for configuration x86_64-w64-windows-gnu.libstdc++.v1.stable.exceptions.nonew, `check-cxx-abilist` will not be available.
-- Configuring done (22.1s)
CMake Error at C:/0/src/llvm-project/libcxx/cmake/Modules/HandleLibCXXABI.cmake:59 (add_library):
  No SOURCES given to target: libcxx-abi-headers-generate-private-headers
Call Stack (most recent call first):
  C:/0/src/llvm-project/libcxx/cmake/Modules/HandleLibCXXABI.cmake:87 (import_private_headers)
  C:/0/src/llvm-project/libcxx/CMakeLists.txt:493 (include)


CMake Generate step failed.  Build files cannot be regenerated correctly.

Could you please help me with my local setup?

As a workaround, I've set it up on my Mac.

pfusik updated this revision to Diff 514848.Apr 19 2023, 12:42 AM

Remove #include <string_view>.
Use the string_view constructor with two pointers.

pfusik updated this revision to Diff 514874.Apr 19 2023, 2:53 AM

Revert to the string_view(ptr, count) constructor.
Hide streambuf::view() from ABI.

pfusik updated this revision to Diff 517514.Apr 27 2023, 4:57 AM

Now submitting using arc.

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.)

libcxx/include/sstream
37

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.

449–457

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.

493

Please format the new code with clang-format so it matches our "normal" style.

libcxx/test/std/input.output/string.streams/istringstream/istringstream.members/str.pass.cpp
14

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.

philnik requested changes to this revision.Apr 27 2023, 10:48 AM
philnik added inline comments.
libcxx/include/sstream
36

Please update the correct status paper in libcxx/docs/Status/.

259

Throughout

499

Generally, we don't spell out the this pointer if not required.

This revision now requires changes to proceed.Apr 27 2023, 10:48 AM

Thanks!

How do I run C++03 tests locally?

libcxx/test/std/input.output/string.streams/istringstream/istringstream.members/str.pass.cpp
14

That would be four tests copied. Why not #if ?

Thanks!

How do I run C++03 tests locally?

Using the argument -Dstd=c++03 for lit.

libcxx/test/std/input.output/string.streams/istringstream/istringstream.members/str.pass.cpp
14

Using a different way makes it harder to find the proper test. When the view test is in str I can't find it and assume it's untested.

The tests you made are not complete for view you need to test whether it's a const member and noexcept so the number of #ifdefs will grow.

For real code duplication is bad, for tests it's not always bad to have duplication. For test easy to understand is more important.

pfusik updated this revision to Diff 517832.Apr 28 2023, 1:16 AM
pfusik marked 6 inline comments as done.

Paper reference, C++20 guards, new tests.

pfusik marked an inline comment as done.Apr 28 2023, 1:20 AM

There's more in the paper. I'd be happy to implement these in subsequent commits.

libcxx/include/sstream
493

How do I do that?

499

Ok. Here it is required (accessing class template base member).

pfusik updated this revision to Diff 517848.Apr 28 2023, 2:36 AM

Add _LIBCPP_HIDE_FROM_ABI

There's more in the paper. I'd be happy to implement these in subsequent commits.

Can you for now add notes to the status page to make clear which parts are not done?

libcxx/include/sstream
493

You can use git clang-format like git clang-format-16 HEAD^ this formats the changes in the last commit.

libcxx/test/std/input.output/string.streams/istringstream/istringstream.members/view.pass.cpp
23 ↗(On Diff #517848)

Why did you add this helper function?

24 ↗(On Diff #517848)

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.

32 ↗(On Diff #517848)

This tests the additional properties of the function.

Is the failed build my fault?

libcxx/test/std/input.output/string.streams/istringstream/istringstream.members/view.pass.cpp
23 ↗(On Diff #517848)

To test const noexcept. I did not know static_assert(noexcept(...)).

pfusik marked an inline comment as done.Apr 28 2023, 10:44 AM

Is the failed build my fault?

No unfortunately these 3 CI runners have issues at the moment.

libcxx/test/std/input.output/string.streams/istringstream/istringstream.members/view.pass.cpp
23 ↗(On Diff #517848)

I think that is quite subtile. I would prefer the making the object const explicitly.

pfusik updated this revision to Diff 519390.May 4 2023, 2:05 AM
pfusik marked an inline comment as done.

Documented paper status.
Removed the get_view helper functions from the tests.

pfusik marked 3 inline comments as done.May 4 2023, 2:24 AM
pfusik added inline comments.
libcxx/include/sstream
493
fox@wiesmac llvm-project % git clang-format HEAD^
clang-format did not modify any files
libcxx/test/std/input.output/string.streams/istringstream/istringstream.members/view.pass.cpp
24 ↗(On Diff #517848)

Now I support your original claim that tests should be simple. There's not much duplication here.

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

libcxx/docs/Status/Cxx20Papers.csv
102 ↗(On Diff #519390)

The note needs to linked to this entry. See remark below.

195 ↗(On Diff #519390)

Here is an example how to link the note.

libcxx/include/sstream
493

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>();
}
497

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.

497

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.

libcxx/test/std/input.output/string.streams/istringstream/istringstream.members/view.pass.cpp
24 ↗(On Diff #517848)

This is something we do in a lot of tests, so I like to keep that pattern.

libcxx/test/std/input.output/string.streams/ostringstream/ostringstream.members/view.pass.cpp
36 ↗(On Diff #519390)

Please also test the const view() returns the proper value. This test would pass with a const member that returns basic_string_view<charT>{}.

pfusik updated this revision to Diff 520319.May 8 2023, 3:35 AM
pfusik marked 7 inline comments as done.

Added a note link.
clang-format new code.
Test with templates on CharT.
Test Traits.

libcxx/include/sstream
497

istringstream exercises the second path. I now added testing the second and third path in the stringbuf test.

pfusik marked 3 inline comments as done.May 8 2023, 3:36 AM
Mordante accepted this revision as: Mordante.May 14 2023, 3:45 AM

LGTM modulo one tiny remark. I leave the final approval to @philnik since he also had comments.

libcxx/test/std/input.output/string.streams/istringstream/istringstream.members/view.pass.cpp
34 ↗(On Diff #520319)

Just for completeness, please apply to all tests.

pfusik updated this revision to Diff 521988.May 14 2023, 4:09 AM

Test noexcept with a non-const object.

pfusik marked an inline comment as done.May 14 2023, 4:10 AM
philnik accepted this revision.May 16 2023, 10:11 AM
philnik added inline comments.
libcxx/include/sstream
493

git-clang-format doesn't recognize sstream because it doesn't have a file ending.

This revision is now accepted and ready to land.May 16 2023, 10:11 AM
This revision was landed with ongoing or failed builds.May 16 2023, 12:03 PM
This revision was automatically updated to reflect the committed changes.