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

[Christopher Di Bella via Phabricator via libcxx-commits]

cjdb added a subscriber: tcanens.
cjdb added inline comments.


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


================
Comment at: libcxx/include/iterator:59
+
+// [iterator.concept.inc], concept incrementable
+template<class I>
----------------
ldionne wrote:
> 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.
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.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100080