[libcxx-commits] [PATCH] D100080: [libcxx][iterator] adds `std::weakly_incrementable` and `std::incrementable`

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision.
ldionne added a comment.
This revision is now accepted and ready to land.

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



================
Comment at: libcxx/include/__iterator/concepts.h:62
+template<class _Tp>
+inline constexpr bool __is_signed_integer_like = signed_integral<_Tp>;
+
----------------
cjdb wrote:
> ldionne wrote:
> > 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?
> 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.
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100080/new/

https://reviews.llvm.org/D100080