[libcxx-commits] [PATCH] D99141: [libcxx] adds `std::incrementable_traits` to <iterator>

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

Can you also make sure the patch passes CI?



================
Comment at: libcxx/include/iterator:16
 
 namespace std
 {
----------------
Can you add the required includes?
http://eel.is/c++draft/iterator.synopsis
```
#include <compare>              // see [compare.syn]
#include <concepts>             // see [concepts.syn]
```


================
Comment at: libcxx/include/iterator:427
 #include <version>
+#include <concepts>
 
----------------
Maybe also add compare while you're at it.


================
Comment at: libcxx/include/iterator:457
+  requires(const _Tp& __x, const _Tp& __y) {
+    { __x - __y } -> integral;
+  };
----------------
cjdb wrote:
> Quuxplusone wrote:
> > This is the //only// reason `<iterator>` needs to include `<concepts>` in C++20 mode, right?
> > @ldionne, how about we just use `is_integral_v` here instead?
> > ```
> > template<class _Tp>
> > concept __has_integral_minus =
> >   !__has_difference_type_member<_Tp> &&
> >   requires(const _Tp& __x, const _Tp& __y) {
> >     requires is_integral_v<decltype( __x - __y )>;
> >   };
> > ```
> > 
> > (And/or, wait for `<concepts>` to stop depending on `<iterator>`; D99041, D99124.)
> > 
> > I note that the name `__has_integral_minus` is slightly misleading; it currently means `__has_integral_minus_without_difference_type`. The paper standard doesn't factor out these concepts, so we have no exposition-only names to imitate here. Maybe `__incrementable_traits_use_difference_type<_Tp>` and `__incrementable_traits_use_minus<_Tp>` would be better? But I don't really object to the current name even if I //do// find it "slightly misleading."
> > 
> > This is the only reason <iterator> needs to include <concepts> in C++20 mode, right?
> 
> For this patch? Yes.
> 
> It's a part of the [[ http://wg21.link/iterator.synopsis | synopsis ]], so in general, no.
> 
> > (And/or, wait for <concepts> to stop depending on <iterator>; D99041, D99124.)
> 
> That was my intention, hence "Depends on D99041".
> 
> > The paper standard doesn't factor out these concepts, so we have no exposition-only names to imitate here. Maybe `__incrementable_traits_use_difference_type<_Tp>` and `__incrementable_traits_use_minus<_Tp>` would be better?
> 
> Right. I think I got the idea of naming these from [[ http://wg21.link/readable.traits | readable.traits ]], so `__has_member_difference_type` (sic) is //probably// best for consistency. As for `__has_integral_minus`, I'm ambivalent, so long as it's consistent.
The synopsis requires concepts http://eel.is/c++draft/iterator.synopsis


================
Comment at: libcxx/test/std/iterators/iterator.requirements/iterator.assoc.types/incrementable.traits/incrementable_traits.pass.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
http://eel.is/c++draft/incrementable.traits#3 `Users may specialize incrementable_­traits on program-defined types.`
can you add a test for this?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99141