This is an archive of the discontinued LLVM Phabricator instance.

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

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


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


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.


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


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

Please avoid reopening namespace std. Just

struct DoubleSentinel {
    friend bool operator==(DoubleSentinel, double*);
    friend int operator-(DoubleSentinel, double*);
    friend int operator-(double*, DoubleSentinel);
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*>);

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


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

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.

SGTM. Will do.

  • Apply Arthur's feedback.

Inline and remove check_sized_sentinel_for.

cjdb added inline comments.Apr 15 2021, 1:00 PM

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!


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


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


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


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


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


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.

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

zoecarver added inline comments.Apr 21 2021, 3:08 PM

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.


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


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


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

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.


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.


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.