[libcxx-commits] [PATCH] D103493: [libcxx][ranges] Add concepts in range.utility.helpers.

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

Quuxplusone added a subscriber: tcanens.
Quuxplusone added a comment.

LGTM % comments.



================
Comment at: libcxx/include/__iterator/concepts.h:172-173
+
+template<class _Iter>
+concept __has_arrow = input_iterator<_Iter> && (is_pointer_v<_Iter> || requires(_Iter __i) { __i.operator->(); });
+
----------------
Nit: Please use `_Ip` not `_Iter`, for consistency.


================
Comment at: libcxx/include/__iterator/iterator_traits.h:257-261
 template<class _Ip>
-concept __has_arrow =
+concept __has_arrow_op =
   requires(_Ip& __i) {
     __i.operator->();
   };
----------------
Before this change: C++20 uses the name `has-arrow` for something complicated; libc++ uses `__has_arrow` for a subtly different thing. This is obviously awful and I'm glad you're changing it.
After this change: C++20 uses the name `has-arrow`; libc++ uses `__has_arrow` for the same thing; libc++ //also// uses `__has_arrow_op` for a subtly different thing. This is arguably a step in the right direction, but still awful, because now it subtly matters whether you say `__has_arrow` or `__has_arrow_op`.

I think the right thing to do here is actually //eliminate `__has_arrow_op` altogether.// We use it in only one place. It's no hardship to inline it on line 264:
```
template<class _Ip>
  requires (requires(_Ip& __i) { __i.operator->(); }) && (!__has_member_pointer<_Ip>)
```
As a drive-by, you might also rename `__iterator_traits_member_pointer_or_arrow_or_void` to simply `__iterator_traits_member_pointer`, for consistency with `__iterator_traits_member_reference`. 


================
Comment at: libcxx/test/libcxx/ranges/range.utility.helpers/has_arrow.compile.pass.cpp:77
+static_assert(std::__has_arrow<E*>);
+static_assert(!std::__has_arrow<Incomplete*>);
+static_assert(std::__has_arrow<simple_iter>);
----------------
This surprises me, but I suppose it's because `Incomplete*` is not an input iterator, because `++it` is ill-formed, because `Incomplete` is incomplete?


================
Comment at: libcxx/test/libcxx/ranges/range.utility.helpers/not_same.compile.pass.cpp:15-21
+static_assert(std::__not_same_as<int, char>);
+static_assert(std::__not_same_as<const int, char>);
+static_assert(!std::__not_same_as<const int, int>);
+static_assert(!std::__not_same_as<const volatile int, int>);
+static_assert(!std::__not_same_as<const int&, int>);
+static_assert(!std::__not_same_as<int&, int>);
+static_assert(!std::__not_same_as<int&&, int>);
----------------
As I think @tcanens remarked a few days ago: WG21 has an exposition-only concept `not-same-as` that is not the same thing as `not same_as`? That's just awful.
Due to its awfulness, I would suggest either giving this a scarier name, or dropping it altogether if humanly possible. (And/or filing a LWG issue, of course.)

If you keep this test, you should add
```
static_assert(!std::__not_same_as<int, int&>);
static_assert(!std::__not_same_as<int&, const int&>);
static_assert(!std::__not_same_as<int(&)(), int()>);
static_assert(std::__not_same_as<int(&)(), int(*)()>);
```
i.e., test some things where the ref-qualification is on the right-hand operand; and test some things where `remove_cvref_t` is visibly different from `decay_t`.


================
Comment at: libcxx/test/libcxx/ranges/range.utility.helpers/simple_view.compile.pass.cpp:22
+
+struct ConstableView : std::ranges::view_base {
+  friend       int* begin(ConstableView&);
----------------
I suggest calling this `WrongConstView` or something, since if you ever saw this in real life it would be a bug.
Initially I thought `SimpleView` should be called `Span` (since it works like `std::span`), but that was when I was expecting the rest of the examples to be different positive examples (e.g. `StringView`) instead of negative examples.


================
Comment at: libcxx/test/libcxx/ranges/range.utility.helpers/simple_view.compile.pass.cpp:29-32
+struct NoConstView : std::ranges::view_base {
+  friend int* begin(NoConstView&);
+  friend int* end(NoConstView&);
+};
----------------
I suggest you add a positive example `OnlyConstView`, with only `friend const int* begin(const OnlyConstView&)`.
And then you can eliminate lines 40 and 42 from `DifferentSentinel`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103493