This is an archive of the discontinued LLVM Phabricator instance.

[libcxx][iterator] adds `std::weakly_incrementable` and `std::incrementable`
ClosedPublic

Authored by cjdb on Apr 7 2021, 5:48 PM.

Details

Summary

Implements parts of:

  • P0896R4 The One Ranges Proposal`

Depends on D100073.

Diff Detail

Event Timeline

cjdb requested review of this revision.Apr 7 2021, 5:48 PM
cjdb created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptApr 7 2021, 5:48 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
cjdb updated this revision to Diff 335967.Apr 7 2021, 5:55 PM

renames all the cryptic types imported from cjdb-ranges

cjdb updated this revision to Diff 336132.Apr 8 2021, 8:43 AM

corrects typo tanking CI

cjdb updated this revision to Diff 336700.Apr 11 2021, 2:40 PM

updates synopsis, rebases to activate CI

cjdb updated this revision to Diff 337040.Apr 12 2021, 9:36 PM

adds subsumption tests

zoecarver accepted this revision as: zoecarver.Apr 14 2021, 3:25 PM

This LGTM sans the small nits.

libcxx/include/iterator
2579

I assume this wasn't intentional.

libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.inc/incrementable.compile.pass.cpp
201

(Non-blocking.) Does it really make sense to test vector and optional, particularly? It seems pretty obvious that these aren't incrementable.

libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.winc/weakly_incrementable.compile.pass.cpp
52

You test this exact same thing in the other file. Let's only test it in one place (this file makes more sense to me than the other one).

213

Same comment.

cjdb updated this revision to Diff 337810.Apr 15 2021, 9:54 AM

rebases to activate CI

cjdb updated this revision to Diff 339029.Apr 20 2021, 4:13 PM
cjdb retitled this revision from [libcxx] adds `std::weakly_incrementable` and `std::incrementable` to <iterator> to [libcxx][iterator] adds `std::weakly_incrementable` and `std::incrementable`.
cjdb edited the summary of this revision. (Show Details)

rebases to activate CI

cjdb updated this revision to Diff 339088.Apr 20 2021, 8:30 PM

rebases to activate CI

cjdb updated this revision to Diff 339276.Apr 21 2021, 9:39 AM

krebases to activate CI

This LGTM generally speaking, but I'd like to understand the story for signed-integer-class types.

libcxx/include/__iterator/concepts.h
62 ↗(On Diff #339276)

From http://eel.is/c++draft/iterator.concept.winc#11:

An integer-like type I is signed-integer-like if it models signed_­integral<I> or if it is a signed-integer-class type.

Here, we detect if it models signed_­integral, but not if it's a signed-integer-class type. Reading the description of what a signed-integer-class type is (starting at http://eel.is/c++draft/iterator.concept.winc#2), it's not clear to me how to even check for that. Do you know what's the intent of the standard here?

libcxx/include/iterator
75

I think those comments could be dropped since they don't add much IMO. Feel free to leave in this patch and we'll address that as a NFC on the whole file later.

cjdb added a subscriber: tcanens.Apr 21 2021, 11:46 AM
cjdb added inline comments.
libcxx/include/__iterator/concepts.h
62 ↗(On Diff #339276)

http://wg21.link/p1522 conveys the full discussion, but the tl;dr is that LWG were concerned about the distances causing overflow between two iterators in the same range (e.g. iota_view). Implementers (and only implementers) are free to provide integer class types with an range larger than long long. We don't do this (our __int128_t is an extended integral type), so we can get away with this definition (which I learnt in Belfast is allowed, cc @tcanens).

Implementing __is_signed_integer_like to account for class types is computationally complex and really error-prone to get right (similar to the old std::boolean concept), so I'd prefer not to do it without first having a class type that we care about.

libcxx/include/iterator
75

Let's leave it for an NFC, for consistency's sake? (i.e. I plan to add them to all future iterator concept patches, and then we can delete them in one fell swoop.)

ldionne accepted this revision.Apr 21 2021, 2:07 PM

I like that we're splitting the conformance tests into their own files like iterator_concept_conformance.compile.pass.cpp.

Ship it once you fix the CI, which fails in a few places (e.g. https://buildkite.com/llvm-project/libcxx-ci/builds/2701#e2424e5f-d53f-4f92-a971-328527c80682).

libcxx/include/__iterator/concepts.h
62 ↗(On Diff #339276)

Thanks a lot for the reference! So it does look like we indeed don't need to do anything here unless/until we decide to add a user-defined integer-class type.

This revision is now accepted and ready to land.Apr 21 2021, 2:07 PM
cjdb updated this revision to Diff 340075.Apr 23 2021, 9:22 AM

rebases to activate CI

cjdb updated this revision to Diff 340223.Apr 23 2021, 7:07 PM

rebases to get Windows CI fixed