Page MenuHomePhabricator

[libcxx][ranges] Add `contiguous_iterator`.
ClosedPublic

Authored by zoecarver on Apr 27 2021, 1:12 PM.

Details

Diff Detail

Unit TestsFailed

TimeTest
550 mslibcxx CI C++20 > libc++.std/iterators/iterator_requirements/iterator_concepts/iterator_concept_random_access::contiguous_iterator.compile.pass.cpp
Script: -- : 'COMPILED WITH'; /usr/bin/clang++ /home/libcxx-builder/.buildkite-agent/builds/6c2ede06786c-1/llvm-project/libcxx-ci/libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.random.access/contiguous_iterator.compile.pass.cpp -v --target=x86_64-unknown-linux-gnu -include /home/libcxx-builder/.buildkite-agent/builds/6c2ede06786c-1/llvm-project/libcxx-ci/libcxx/test/support/nasty_macros.h -nostdinc++ -I/home/libcxx-builder/.buildkite-agent/builds/6c2ede06786c-1/llvm-project/libcxx-ci/build/generic-cxx20/include/c++/v1 -I/home/libcxx-builder/.buildkite-agent/builds/6c2ede06786c-1/llvm-project/libcxx-ci/build/generic-cxx20/projects/libcxx/include/c++build -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -D__STDC_CONSTANT_MACROS -I/home/libcxx-builder/.buildkite-agent/builds/6c2ede06786c-1/llvm-project/libcxx-ci/libcxx/test/support -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -std=c++20 -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-atomic-alignment -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_DISABLE_AVAILABILITY -fcoroutines-ts -Werror=thread-safety -Wuser-defined-warnings -fmodules-cache-path=/home/libcxx-builder/.buildkite-agent/builds/6c2ede06786c-1/llvm-project/libcxx-ci/build/generic-cxx20/projects/libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.random.access/Output/contiguous_iterator.compile.pass.cpp.dir/t.tmp/ModuleCache -Wno-macro-redefined -D_LIBCPP_HAS_THREAD_API_PTHREAD -Wno-macro-redefined -D_LIBCPP_ABI_VERSION=1 -fsyntax-only
710 mslibcxx CI C++2b > libc++.std/iterators/iterator_requirements/iterator_concepts/iterator_concept_random_access::contiguous_iterator.compile.pass.cpp
Script: -- : 'COMPILED WITH'; /usr/bin/clang++-tot /home/libcxx-builder/.buildkite-agent/builds/59e01e748de0-1/llvm-project/libcxx-ci/libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.random.access/contiguous_iterator.compile.pass.cpp -v --target=x86_64-unknown-linux-gnu -include /home/libcxx-builder/.buildkite-agent/builds/59e01e748de0-1/llvm-project/libcxx-ci/libcxx/test/support/nasty_macros.h -nostdinc++ -I/home/libcxx-builder/.buildkite-agent/builds/59e01e748de0-1/llvm-project/libcxx-ci/build/generic-cxx2b/include/c++/v1 -I/home/libcxx-builder/.buildkite-agent/builds/59e01e748de0-1/llvm-project/libcxx-ci/build/generic-cxx2b/projects/libcxx/include/c++build -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -D__STDC_CONSTANT_MACROS -I/home/libcxx-builder/.buildkite-agent/builds/59e01e748de0-1/llvm-project/libcxx-ci/libcxx/test/support -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -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-atomic-alignment -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_DISABLE_AVAILABILITY -fcoroutines-ts -Werror=thread-safety -Wuser-defined-warnings -fmodules-cache-path=/home/libcxx-builder/.buildkite-agent/builds/59e01e748de0-1/llvm-project/libcxx-ci/build/generic-cxx2b/projects/libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.random.access/Output/contiguous_iterator.compile.pass.cpp.dir/t.tmp/ModuleCache -Wno-macro-redefined -D_LIBCPP_HAS_THREAD_API_PTHREAD -Wno-macro-redefined -D_LIBCPP_ABI_VERSION=1 -fsyntax-only

Event Timeline

zoecarver requested review of this revision.Apr 27 2021, 1:12 PM
zoecarver created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptApr 27 2021, 1:12 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.random.access/contiguous_iterator.compile.pass.cpp
28–32

Most importantly, please check the test iterators from test_iterators.h — e.g.

static_assert(std::contiguous_iterator<contiguous_iterator<int*>>);

Any test iterators that don't match the concepts will want to be fixed.

zoecarver added inline comments.Apr 27 2021, 1:55 PM
libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.random.access/contiguous_iterator.compile.pass.cpp
28–32

I have those a few lines above:

static_assert(!std::contiguous_iterator<input_iterator<int*> >);
static_assert(!std::contiguous_iterator<forward_iterator<int*> >);
static_assert(!std::contiguous_iterator<bidirectional_iterator<int*> >);
static_assert(!std::contiguous_iterator<random_access_iterator<int*> >);
static_assert(std::contiguous_iterator<contiguous_iterator<int*> >);
cjdb accepted this revision.Apr 27 2021, 3:01 PM

Missing conformance tests:

  • deque (negative test)
  • vector
  • span
  • string
  • reverse_iterator

LGTM, pending CI and added conformance tests.

zoecarver planned changes to this revision.May 6 2021, 11:33 AM
zoecarver updated this revision to Diff 344172.May 10 2021, 1:15 PM
  • Add conformance tests.
  • Fix review comments.
  • Detatch from contiguous_range.
zoecarver retitled this revision from [libcxx][ranges] Add `contiguous_{iterator,range}`. to [libcxx][ranges] Add `contiguous_iterator`..May 10 2021, 1:16 PM
zoecarver added inline comments.
libcxx/include/__memory/pointer_traits.h
201 ↗(On Diff #344172)

I'm going to make this into a separate PR with test.

zoecarver updated this revision to Diff 344175.May 10 2021, 1:19 PM
  • Grep/fix the last few conformance tests.
zoecarver updated this revision to Diff 344177.May 10 2021, 1:20 PM
  • Remove final remaining range test.

(Sorry for all the churn/emails!)

zoecarver updated this revision to Diff 344184.May 10 2021, 1:31 PM
  • Rebase on main to get the to_address fix.
zoecarver updated this revision to Diff 344185.May 10 2021, 1:34 PM
  • Reset changes in pointer_traits to fix the diff.

Fixing the build failures now. @Quuxplusone friendly ping :)

zoecarver updated this revision to Diff 345289.May 13 2021, 2:34 PM

Fix the two failing tests.

libcxx/test/std/containers/sequences/vector.bool/iterator_concept_conformance.compile.pass.cpp
27

Just want to confirm this is correct: vector<bool> is not a contiguous container (while normal vector is).

cjdb added inline comments.May 13 2021, 2:40 PM
libcxx/test/std/containers/sequences/vector.bool/iterator_concept_conformance.compile.pass.cpp
27

Correct.

LGTM % comments

libcxx/include/__iterator/concepts.h
15

At some point: Alphabetize.

libcxx/test/std/containers/sequences/vector.bool/iterator_concept_conformance.compile.pass.cpp
27

Please keep static_assert(std::random_access_iterator<reverse_iterator>); (just add the new negative asserts).

libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.random.access/contiguous_iterator.compile.pass.cpp
15

concept contiguous_iterator;

46–50

(A) this could be operator<=>?
(B) pass-by-const-value alert! I think you wanted pass-by-const-reference.
Ditto throughout.

libcxx/test/std/iterators/predef.iterators/reverse.iterators/iterator_concept_conformance.compile.pass.cpp
42–43

Please keep the assert for

static_assert(std::random_access_iterator<reverse_contiguous_iterator>);

(just add the new negative assert). This also applies anywhere else in this PR that you removed a positive assert without replacing it with a stronger positive assert (if any such places exist).

zoecarver updated this revision to Diff 345462.May 14 2021, 9:22 AM

Apply Arthur's comments.

zoecarver updated this revision to Diff 345463.May 14 2021, 9:25 AM

Remove extra requires that's no longer needed now that to_address is SFINAE friendly.

Add back SFINAE fix.

This revision was not accepted when it landed; it landed in state Needs Review.May 14 2021, 3:28 PM
This revision was automatically updated to reflect the committed changes.