This is an archive of the discontinued LLVM Phabricator instance.

[libc++][ranges] iterator.concept.sizedsentinel: sized_sentinel_for and disable_sized_sentinel_for.
ClosedPublic

Authored by zoecarver on Apr 15 2021, 11:35 AM.

Diff Detail

Event Timeline

zoecarver requested review of this revision.Apr 15 2021, 11:35 AM
zoecarver created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptApr 15 2021, 11:35 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
  • Small formatting changes.
cjdb accepted this revision.Apr 15 2021, 11:39 AM

LGTM

miscco accepted this revision.Apr 15 2021, 11:48 AM
miscco added a subscriber: miscco.

LGTM

zoecarver accepted this revision as: Restricted Project.Apr 15 2021, 11:50 AM

Thanks! I'll land it once the tests pass and parent patches are committed.

This revision is now accepted and ready to land.Apr 15 2021, 11:50 AM
Quuxplusone requested changes to this revision.Apr 15 2021, 11:56 AM

Code looks good modulo naming; the test drags in a lot of extra dependencies it doesn't need.

libcxx/include/iterator
2600–2607

I'd prefer to see _Sp and _Ip just for consistency with line 2591.

libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.sentinel/sized_sentinel_for.compile.pass.cpp
42–47

Instead of std::input_or_output_iterator auto, how about int*:

struct IntSentinel {
    friend bool operator==(IntSentinel, int*);
    friend int operator-(IntSentinel, int*);
    friend int operator-(int*, IntSentinel);
};
static_assert(std::sized_sentinel_for<IntSentinel, int*>);
static_assert(!std::sized_sentinel_for<int*, IntSentinel>);  // IntSentinel is not an iterator
65–71

Please avoid reopening namespace std. Just

struct DoubleSentinel {
    friend bool operator==(DoubleSentinel, double*);
    friend int operator-(DoubleSentinel, double*);
    friend int operator-(double*, DoubleSentinel);
};
template<>
inline constexpr bool std::disable_sized_sentinel_for<DoubleSentinel, double*> = true;
    // notice the un-necessity of `namespace std {` here

static_assert(!std::sized_sentinel_for<DoubleSentinel, double*>);
155–162

I don't think this test should depend on <filesystem>. (As below, all you're testing here is that you can't subtract directory_iterators, and we knew that already.)

186–190

These initially confused me, but then I got it: it's because these iterators are merely bidirectional, not random-access, so they don't support operator-, so they're not "sized" sentinels. I would prefer to eliminate the dependency on the associative containers and write simply e.g.

#include "test_iterators.h"
static_assert(std::sized_sentinel_for<random_access_iterator<int*>, random_access_iterator<int*>>);
static_assert(!std::sized_sentinel_for<bidirectional_iterator<int*>, bidirectional_iterator<int*>>);
This revision now requires changes to proceed.Apr 15 2021, 11:56 AM
cjdb added inline comments.Apr 15 2021, 12:13 PM
libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.sentinel/sized_sentinel_for.compile.pass.cpp
186–190

Reiterating from previous reviews, which this one has inherited from: the point is to make sure these types are standard conforming. The dependencies are intentional.

zoecarver marked an inline comment as done.Apr 15 2021, 12:31 PM
zoecarver added inline comments.
libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.sentinel/sized_sentinel_for.compile.pass.cpp
186–190

SGTM. Will do.

  • Apply Arthur's feedback.

Inline and remove check_sized_sentinel_for.

cjdb added inline comments.Apr 15 2021, 1:00 PM
libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.sentinel/sized_sentinel_for.compile.pass.cpp
155–162

and we knew that already

Which existing tests confirm this isn't possible?

@Quuxplusone does this look OK to you now?

Quuxplusone accepted this revision.Apr 20 2021, 10:20 AM

LGTM % comments — fix the nits and land it!

libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.sentinel/sized_sentinel_for.compile.pass.cpp
40–42

Please remove constexpr.
Ditto lines 74-76 and lines 82-86.

45–47

Nit: Either break the line between ; and //, or simply s/int_sized_sentinel/IntSentinel/ so that you don't need to break the line.

54

s/std::vector<int>::iterator/int*/; then you don't need to break the line.
Ditto on line 60.

57

Add: not_copyable() = default;
Otherwise, this isn't significantly different from the no_default_ctor case. (Remember, a class with any declared ctors doesn't get a defaulted default ctor, unless you explicitly default the default ctor.)

110–111

Remove this linebreak. Ditto through the end of the file.

131–132

s/std::vector<int>::iterator/int*/g so that you don't have to break the line.

This revision is now accepted and ready to land.Apr 20 2021, 10:20 AM
cjdb requested changes to this revision.Apr 20 2021, 4:26 PM

Please move all iterator concept conformance tests into their appropriate files (including re-adding any iterators that might have been deleted previously).

This revision now requires changes to proceed.Apr 20 2021, 4:26 PM
zoecarver updated this revision to Diff 339398.Apr 21 2021, 3:03 PM
  • Use new test structure.
zoecarver updated this revision to Diff 339402.Apr 21 2021, 3:06 PM
  • Unindent comment that got moved.
libcxx/include/__iterator/concepts.h
111 ↗(On Diff #339398)

Any ideas as to why clang format wants to move this comment?

zoecarver added inline comments.Apr 21 2021, 3:08 PM
libcxx/include/__iterator/concepts.h
111 ↗(On Diff #339402)

Any ideas as to why clang format wants to move this comment?

This diff moved the phab comment 🤦. This is the code comment I'm talking about.

cjdb accepted this revision.Apr 21 2021, 3:35 PM

std::list hasn't had its conformance test updated, but otherwise I have one nit.

libcxx/test/std/containers/associative/map/iterator_concept_conformance.compile.pass.cpp
33–34 ↗(On Diff #339402)

This should be for iterator, and is missing checks for iterator and const_iterator on the RHS. Same for other files too.

44–45 ↗(On Diff #339402)

Missing checks for iterator and const_iterator on the RHS. Same for other files too.

libcxx/test/std/containers/sequences/array/iterator_concept_conformance.compile.pass.cpp
42–60 ↗(On Diff #339402)

Could we order these as if they were a set of increasing two digit numbers please?
Where

iterator: 0
const_iterator: 1
reverse_iterator: 2
const_reverse_iterator: 3

I think that will make it easier to track that all have been accounted for in the right permutation.

libcxx/test/std/containers/unord/unord.map/iterator_concept_conformance.compile.pass.cpp
61 ↗(On Diff #339402)

local_iterator and const_local_iterator need checking too please.

This revision is now accepted and ready to land.Apr 21 2021, 3:35 PM
zoecarver updated this revision to Diff 339417.Apr 21 2021, 3:53 PM
  • Ensure line break for every changed file.
zoecarver updated this revision to Diff 339462.Apr 21 2021, 9:36 PM
  • Add, fix, and reorder tests.

I'll land as soon as the two parent patches are committed.

cjdb added a comment.Apr 21 2021, 9:58 PM

std::list still hasn't had its conformance test updated.

libcxx/test/std/containers/sequences/deque/iterator_concept_conformance.compile.pass.cpp
53–61 ↗(On Diff #339462)

Thos lot isn't necessary (std::reverse_iterator gets its own file so it doesn't need to be on the LHS, but it looks to me like that file is missing?).

  • Remove reverse iterator tests.
  • Add a test in reverse.iterator.

std::list still hasn't had its conformance test updated.

For any future reviewers: this is done.

ldionne accepted this revision.Apr 22 2021, 11:13 AM
zoecarver updated this revision to Diff 340565.Apr 26 2021, 9:40 AM
  • Rebase.
  • Fix 32bit bots.
This revision was landed with ongoing or failed builds.Apr 26 2021, 3:06 PM
This revision was automatically updated to reflect the committed changes.

By the way, the windows bots were failing (everything else passed) because I didn't have an XFAIL on the added test but I added that before committing.