Page MenuHomePhabricator

[libcxx][nfc] Cleanup cpp20_input_iterator.
Changes PlannedPublic

Authored by Quuxplusone on May 28 2021, 1:58 PM.

Details

Reviewers
cjdb
ldionne
zoecarver
Group Reviewers
Restricted Project
Summary

As requested by Arthur in D102020.

Diff Detail

Unit TestsFailed

TimeTest
2,009 mslibcxx CI GCC-next/C++20 > libc++.libcxx/input_output/filesystems/class_path/path_req::is_pathable.pass.cpp
Script: -- : 'COMPILED WITH'; /usr/bin/g++-11 /home/libcxx-builder/.buildkite-agent/builds/eec7faa1d122-1/llvm-project/libcxx-ci/libcxx/test/libcxx/input.output/filesystems/class.path/path.req/is_pathable.pass.cpp -v -include /home/libcxx-builder/.buildkite-agent/builds/eec7faa1d122-1/llvm-project/libcxx-ci/libcxx/test/support/nasty_macros.h -nostdinc++ -I/home/libcxx-builder/.buildkite-agent/builds/eec7faa1d122-1/llvm-project/libcxx-ci/build/generic-gcc-next/include/c++/v1 -I/home/libcxx-builder/.buildkite-agent/builds/eec7faa1d122-1/llvm-project/libcxx-ci/build/generic-gcc-next/projects/libcxx/include/c++build -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -D__STDC_CONSTANT_MACROS -I/home/libcxx-builder/.buildkite-agent/builds/eec7faa1d122-1/llvm-project/libcxx-ci/libcxx/test/support -std=c++2b -Werror -Wall -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-user-defined-literals -Wno-noexcept-type -Wno-aligned-allocation-unavailable -Wno-atomic-alignment -Wno-sized-deallocation -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_DISABLE_AVAILABILITY -Wno-macro-redefined -D_LIBCPP_HAS_THREAD_API_PTHREAD -Wno-macro-redefined -D_LIBCPP_ABI_VERSION=1 -lc++experimental -L/home/libcxx-builder/.buildkite-agent/builds/eec7faa1d122-1/llvm-project/libcxx-ci/build/generic-gcc-next/./lib -Wl,-rpath,/home/libcxx-builder/.buildkite-agent/builds/eec7faa1d122-1/llvm-project/libcxx-ci/build/generic-gcc-next/./lib -L/home/libcxx-builder/.buildkite-agent/builds/eec7faa1d122-1/llvm-project/libcxx-ci/build/generic-gcc-next/./lib -Wl,-rpath,/home/libcxx-builder/.buildkite-agent/builds/eec7faa1d122-1/llvm-project/libcxx-ci/build/generic-gcc-next/./lib -nodefaultlibs -lc++ -lm -lgcc_s -lgcc -lpthread -lc -lgcc_s -lgcc -latomic -o /home/libcxx-builder/.buildkite-agent/builds/eec7faa1d122-1/llvm-project/libcxx-ci/build/generic-gcc-next/projects/libcxx/test/libcxx/input.output/filesystems/class.path/path.
1,430 mslibcxx CI GCC-next/C++20 > libc++.libcxx/iterators::advance.debug1.pass.cpp
Script: -- : 'COMPILED WITH'; /usr/bin/g++-11 /home/libcxx-builder/.buildkite-agent/builds/eec7faa1d122-1/llvm-project/libcxx-ci/libcxx/test/libcxx/iterators/advance.debug1.pass.cpp -v -include /home/libcxx-builder/.buildkite-agent/builds/eec7faa1d122-1/llvm-project/libcxx-ci/libcxx/test/support/nasty_macros.h -nostdinc++ -I/home/libcxx-builder/.buildkite-agent/builds/eec7faa1d122-1/llvm-project/libcxx-ci/build/generic-gcc-next/include/c++/v1 -I/home/libcxx-builder/.buildkite-agent/builds/eec7faa1d122-1/llvm-project/libcxx-ci/build/generic-gcc-next/projects/libcxx/include/c++build -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -D__STDC_CONSTANT_MACROS -I/home/libcxx-builder/.buildkite-agent/builds/eec7faa1d122-1/llvm-project/libcxx-ci/libcxx/test/support -std=c++2b -Werror -Wall -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-user-defined-literals -Wno-noexcept-type -Wno-aligned-allocation-unavailable -Wno-atomic-alignment -Wno-sized-deallocation -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_DISABLE_AVAILABILITY -Wno-macro-redefined -D_LIBCPP_HAS_THREAD_API_PTHREAD -Wno-macro-redefined -D_LIBCPP_ABI_VERSION=1 -D_LIBCPP_DEBUG=0 -lc++experimental -L/home/libcxx-builder/.buildkite-agent/builds/eec7faa1d122-1/llvm-project/libcxx-ci/build/generic-gcc-next/./lib -Wl,-rpath,/home/libcxx-builder/.buildkite-agent/builds/eec7faa1d122-1/llvm-project/libcxx-ci/build/generic-gcc-next/./lib -L/home/libcxx-builder/.buildkite-agent/builds/eec7faa1d122-1/llvm-project/libcxx-ci/build/generic-gcc-next/./lib -Wl,-rpath,/home/libcxx-builder/.buildkite-agent/builds/eec7faa1d122-1/llvm-project/libcxx-ci/build/generic-gcc-next/./lib -nodefaultlibs -lc++ -lm -lgcc_s -lgcc -lpthread -lc -lgcc_s -lgcc -latomic -o /home/libcxx-builder/.buildkite-agent/builds/eec7faa1d122-1/llvm-project/libcxx-ci/build/generic-gcc-next/projects/libcxx/test/libcxx/iterators/Output/advance.debug1.pass.cpp.dir/t.tmp.exe…
1,420 mslibcxx CI GCC-next/C++20 > libc++.libcxx/iterators::contiguous_iterators.pass.cpp
Script: -- : 'COMPILED WITH'; /usr/bin/g++-11 /home/libcxx-builder/.buildkite-agent/builds/eec7faa1d122-1/llvm-project/libcxx-ci/libcxx/test/libcxx/iterators/contiguous_iterators.pass.cpp -v -include /home/libcxx-builder/.buildkite-agent/builds/eec7faa1d122-1/llvm-project/libcxx-ci/libcxx/test/support/nasty_macros.h -nostdinc++ -I/home/libcxx-builder/.buildkite-agent/builds/eec7faa1d122-1/llvm-project/libcxx-ci/build/generic-gcc-next/include/c++/v1 -I/home/libcxx-builder/.buildkite-agent/builds/eec7faa1d122-1/llvm-project/libcxx-ci/build/generic-gcc-next/projects/libcxx/include/c++build -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -D__STDC_CONSTANT_MACROS -I/home/libcxx-builder/.buildkite-agent/builds/eec7faa1d122-1/llvm-project/libcxx-ci/libcxx/test/support -std=c++2b -Werror -Wall -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-user-defined-literals -Wno-noexcept-type -Wno-aligned-allocation-unavailable -Wno-atomic-alignment -Wno-sized-deallocation -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_DISABLE_AVAILABILITY -Wno-macro-redefined -D_LIBCPP_HAS_THREAD_API_PTHREAD -Wno-macro-redefined -D_LIBCPP_ABI_VERSION=1 -lc++experimental -L/home/libcxx-builder/.buildkite-agent/builds/eec7faa1d122-1/llvm-project/libcxx-ci/build/generic-gcc-next/./lib -Wl,-rpath,/home/libcxx-builder/.buildkite-agent/builds/eec7faa1d122-1/llvm-project/libcxx-ci/build/generic-gcc-next/./lib -L/home/libcxx-builder/.buildkite-agent/builds/eec7faa1d122-1/llvm-project/libcxx-ci/build/generic-gcc-next/./lib -Wl,-rpath,/home/libcxx-builder/.buildkite-agent/builds/eec7faa1d122-1/llvm-project/libcxx-ci/build/generic-gcc-next/./lib -nodefaultlibs -lc++ -lm -lgcc_s -lgcc -lpthread -lc -lgcc_s -lgcc -latomic -o /home/libcxx-builder/.buildkite-agent/builds/eec7faa1d122-1/llvm-project/libcxx-ci/build/generic-gcc-next/projects/libcxx/test/libcxx/iterators/Output/contiguous_iterators.pass.cpp.dir/t.tmp.exe…
1,410 mslibcxx CI GCC-next/C++20 > libc++.libcxx/iterators::next.debug1.pass.cpp
Script: -- : 'COMPILED WITH'; /usr/bin/g++-11 /home/libcxx-builder/.buildkite-agent/builds/eec7faa1d122-1/llvm-project/libcxx-ci/libcxx/test/libcxx/iterators/next.debug1.pass.cpp -v -include /home/libcxx-builder/.buildkite-agent/builds/eec7faa1d122-1/llvm-project/libcxx-ci/libcxx/test/support/nasty_macros.h -nostdinc++ -I/home/libcxx-builder/.buildkite-agent/builds/eec7faa1d122-1/llvm-project/libcxx-ci/build/generic-gcc-next/include/c++/v1 -I/home/libcxx-builder/.buildkite-agent/builds/eec7faa1d122-1/llvm-project/libcxx-ci/build/generic-gcc-next/projects/libcxx/include/c++build -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -D__STDC_CONSTANT_MACROS -I/home/libcxx-builder/.buildkite-agent/builds/eec7faa1d122-1/llvm-project/libcxx-ci/libcxx/test/support -std=c++2b -Werror -Wall -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-user-defined-literals -Wno-noexcept-type -Wno-aligned-allocation-unavailable -Wno-atomic-alignment -Wno-sized-deallocation -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_DISABLE_AVAILABILITY -Wno-macro-redefined -D_LIBCPP_HAS_THREAD_API_PTHREAD -Wno-macro-redefined -D_LIBCPP_ABI_VERSION=1 -D_LIBCPP_DEBUG=0 -lc++experimental -L/home/libcxx-builder/.buildkite-agent/builds/eec7faa1d122-1/llvm-project/libcxx-ci/build/generic-gcc-next/./lib -Wl,-rpath,/home/libcxx-builder/.buildkite-agent/builds/eec7faa1d122-1/llvm-project/libcxx-ci/build/generic-gcc-next/./lib -L/home/libcxx-builder/.buildkite-agent/builds/eec7faa1d122-1/llvm-project/libcxx-ci/build/generic-gcc-next/./lib -Wl,-rpath,/home/libcxx-builder/.buildkite-agent/builds/eec7faa1d122-1/llvm-project/libcxx-ci/build/generic-gcc-next/./lib -nodefaultlibs -lc++ -lm -lgcc_s -lgcc -lpthread -lc -lgcc_s -lgcc -latomic -o /home/libcxx-builder/.buildkite-agent/builds/eec7faa1d122-1/llvm-project/libcxx-ci/build/generic-gcc-next/projects/libcxx/test/libcxx/iterators/Output/next.debug1.pass.cpp.dir/t.tmp.exe…
1,420 mslibcxx CI GCC-next/C++20 > libc++.libcxx/iterators::prev.debug1.pass.cpp
Script: -- : 'COMPILED WITH'; /usr/bin/g++-11 /home/libcxx-builder/.buildkite-agent/builds/eec7faa1d122-1/llvm-project/libcxx-ci/libcxx/test/libcxx/iterators/prev.debug1.pass.cpp -v -include /home/libcxx-builder/.buildkite-agent/builds/eec7faa1d122-1/llvm-project/libcxx-ci/libcxx/test/support/nasty_macros.h -nostdinc++ -I/home/libcxx-builder/.buildkite-agent/builds/eec7faa1d122-1/llvm-project/libcxx-ci/build/generic-gcc-next/include/c++/v1 -I/home/libcxx-builder/.buildkite-agent/builds/eec7faa1d122-1/llvm-project/libcxx-ci/build/generic-gcc-next/projects/libcxx/include/c++build -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -D__STDC_CONSTANT_MACROS -I/home/libcxx-builder/.buildkite-agent/builds/eec7faa1d122-1/llvm-project/libcxx-ci/libcxx/test/support -std=c++2b -Werror -Wall -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-user-defined-literals -Wno-noexcept-type -Wno-aligned-allocation-unavailable -Wno-atomic-alignment -Wno-sized-deallocation -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_DISABLE_AVAILABILITY -Wno-macro-redefined -D_LIBCPP_HAS_THREAD_API_PTHREAD -Wno-macro-redefined -D_LIBCPP_ABI_VERSION=1 -D_LIBCPP_DEBUG=0 -lc++experimental -L/home/libcxx-builder/.buildkite-agent/builds/eec7faa1d122-1/llvm-project/libcxx-ci/build/generic-gcc-next/./lib -Wl,-rpath,/home/libcxx-builder/.buildkite-agent/builds/eec7faa1d122-1/llvm-project/libcxx-ci/build/generic-gcc-next/./lib -L/home/libcxx-builder/.buildkite-agent/builds/eec7faa1d122-1/llvm-project/libcxx-ci/build/generic-gcc-next/./lib -Wl,-rpath,/home/libcxx-builder/.buildkite-agent/builds/eec7faa1d122-1/llvm-project/libcxx-ci/build/generic-gcc-next/./lib -nodefaultlibs -lc++ -lm -lgcc_s -lgcc -lpthread -lc -lgcc_s -lgcc -latomic -o /home/libcxx-builder/.buildkite-agent/builds/eec7faa1d122-1/llvm-project/libcxx-ci/build/generic-gcc-next/projects/libcxx/test/libcxx/iterators/Output/prev.debug1.pass.cpp.dir/t.tmp.exe…
View Full Test Results (777 Failed)

Event Timeline

zoecarver requested review of this revision.May 28 2021, 1:58 PM
zoecarver created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptMay 28 2021, 1:58 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
zoecarver added inline comments.May 28 2021, 1:59 PM
libcxx/test/support/test_iterators.h
639

I need to find and remove the matching comment.

737

We need some similar changes here.

cjdb added a comment.May 28 2021, 2:03 PM

The suggested changes from D102020 don't properly consider the design of this type, nor its surrounding ecosystem. Please do not merge it, and instead use the appropriate helper types that are already in test_iterators.h.

libcxx/test/support/test_iterators.h
636–637

Why? This is a move-only iterator, which only makes sense in ranges land.

641–642

Why?

663

This overload needs to keep its [[nodiscard]].

665–676

Iterators don't have sentinel sub-types. We have a sentinel_wrapper for that.

cjdb added inline comments.May 28 2021, 2:36 PM
libcxx/test/support/test_iterators.h
641–642

Upon further thought, removing this opens a gaping hole for when a I satisfies std::input_iterator, but not _`cpp17-input-iterator`_ and doesn't result in the primary template for iterator_traits.

Quuxplusone added inline comments.May 28 2021, 2:39 PM
libcxx/test/support/test_iterators.h
636–637

It makes sense anywhere it compiles, which is to say >=14.

641–642

'14-compatibility.

663

IMO, no, the [[nodiscard]] was just noise. We don't need to [[nodiscard]] everything in sight: we don't [[nodiscard]] operator*, for example, even though "obviously" any test code that actually discarded the result of *it would probably have a bug, just as much as any test code that actually discarded the result of it.base().

665–676

Hm. I don't particularly dislike sentinel_wrapper. At worst it's a tiny bit more verbose: compare the angle-bracketiness of alternatives

some_function<cpp20_input_iterator<int*>, sentinel_wrapper<cpp20_input_iterator<int*>>>();
some_function<cpp20_input_iterator<int*>, cpp20_input_iterator<int*>::sentinel>();

(Right now sentinel_wrapper is used only in "range.iter.ops.advance/advance.pass.cpp".)

I'm okay eliminating this diff for now, pending some examples of how sentinel{,_wrapper} would be used in real test cases. Certainly if we're adding sentinel we should also be refactoring some existing tests to show how it's an improvement — if it's not an improvement then we shouldn't do it.

872–873

Please void operator&() const DELETE_FUNCTION; while you're at it.

cjdb added inline comments.May 28 2021, 3:08 PM
libcxx/test/support/test_iterators.h
636–637

Yeah, no. This isn't useful in C++14, so it shouldn't be available. Not to mention that it depends on std::iter_value_t and std::iter_difference_t.

665–676

Hm. I don't particularly dislike sentinel_wrapper. At worst it's a tiny bit more verbose: compare the angle-bracketiness of alternatives

sentinel_wrapper is the sentinel version of our iterator wrappers; if your hard disk has a limited amount of free space where each byte counts, you'd be doing yourself a disservice as we'd need to duplicate this code for each of our other iterators too.

(Right now sentinel_wrapper is used only in "range.iter.ops.advance/advance.pass.cpp".)

It's also used in range.iter.ops/range.iter.ops.next. I should fix up range.iter.ops/range.iter.ops.prev to use it too.

I'm okay eliminating this diff for now, pending some examples of how sentinel{,_wrapper} would be used in real test cases.

If, by "real test cases", you mean "more test cases" (those places where it's used today seem real to me): those should start to flow in when algorithms start to get committed.

Certainly if we're adding sentinel we should also be refactoring some existing tests to show how it's an improvement — if it's not an improvement then we shouldn't do it.

It's a bit hard to refactor code that doesn't exist yet, but here's an example of what might exist in the near future.

auto v = std::vector{1, 2, 3};
std::ranges::find(v.begin(), sentinel_wrapper(v.end()), 0);
Quuxplusone commandeered this revision.Jun 1 2021, 8:07 PM
Quuxplusone planned changes to this revision.
Quuxplusone edited reviewers, added: zoecarver; removed: Quuxplusone.