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

[Christopher Di Bella via Phabricator via libcxx-commits]

cjdb added inline comments.


================
Comment at: libcxx/include/iterator:457
+  requires(const _Tp& __x, const _Tp& __y) {
+    { __x - __y } -> integral;
+  };
----------------
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.


================
Comment at: libcxx/include/iterator:467
+struct incrementable_traits<_Tp> {
+  using difference_type = make_signed_t<decltype(std::declval<_Tp>() - std::declval<_Tp>())>;
+};
----------------
Quuxplusone wrote:
> Just `declval<_Tp>()`, not `std::declval<_Tp>()`. (If it had any arguments, then definitely `_VSTD::declval` to prevent ADL — but it has no arguments. Actually we still have a ton of places that do `_VSTD::declval`, so I'm okay with that spelling too. Just not `std::`.)
Eh, ADL isn't a concern here. Removed.


================
Comment at: libcxx/test/std/iterators/iterator.requirements/iterator.assoc.types/incrementable.traits/incrementable_traits.pass.cpp:43
+
+static_assert(check_incrementable_traits<int[], std::ptrdiff_t>());
+
----------------
Quuxplusone wrote:
> This is surprising, but I guess it falls out naturally because `int[]` is subtractable?
> Please add tests for `int[10]`, `int(*)()`, `int(&)()`, and `int()` as well.
See below for `int(*)()` and friends.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99141