This is an archive of the discontinued LLVM Phabricator instance.

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

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

Details

Diff Detail

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
26

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
26

Correct.

LGTM % comments

libcxx/include/__iterator/concepts.h
16

At some point: Alphabetize.

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

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

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.