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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added inline comments.


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



================
Comment at: libcxx/include/iterator:467
+struct incrementable_traits<_Tp> {
+  using difference_type = make_signed_t<decltype(std::declval<_Tp>() - std::declval<_Tp>())>;
+};
----------------
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::`.)


================
Comment at: libcxx/test/std/iterators/iterator.requirements/iterator.assoc.types/incrementable.traits/incrementable_traits.pass.cpp:22
+  typename std::incrementable_traits<T>::difference_type;
+};
+
----------------
Almost all places where you write `incrementable_traits`, you should be writing `difference_type` — `has_difference_type`, `difference_type_matches`, `check_difference_type`.


================
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>());
+
----------------
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.


================
Comment at: libcxx/test/std/iterators/iterator.requirements/iterator.assoc.types/incrementable.traits/incrementable_traits.pass.cpp:158
+  friend int operator-(rvalue_ref_with_cv_subtraction&&,
+                       rvalue_ref_with_cv_subtraction&&) noexcept;
+};
----------------
"with_cv"? I see no cv-qualifiers here.
Also, my usual opinion on `noexcept` here: `noexcept` is absolutely irrelevant to this test, and so it would be nice not to see it at all. At the very least, please make //at least one// of these tests check what happens when the subtraction operation is non-noexcept, because that's the realistic case in the wild.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99141