[libcxx-commits] [PATCH] D99873: [libcxx] adds `std::ranges::iter_move` and `std::iter_rvalue_reference_t

[Tim Song via Phabricator via libcxx-commits]

tcanens added inline comments.


================
Comment at: libcxx/include/iterator:2438
+template<class _Ip>
+void iter_move(_Ip);
+
----------------
This doesn't match the standard. The "poison pill" that doesn't poison anything is there solely to clarify that we only do ADL.


================
Comment at: libcxx/include/iterator:2441
+template<class _Ip>
+concept __unqualified_iter_move =
+  __dereferenceable<_Ip> &&
----------------
This is missing the "`E` has class or enumeration type" part of the spec. In particular, we don't want to do ADL for pointers.


================
Comment at: libcxx/include/iterator:2442
+concept __unqualified_iter_move =
+  __dereferenceable<_Ip> &&
+  requires(_Ip& __i) {
----------------
This is not in the spec?


================
Comment at: libcxx/include/iterator:2444
+  requires(_Ip& __i) {
+    { iter_move(__i) } -> __can_reference;
+  };
----------------
nor is the `__can_reference` here.


================
Comment at: libcxx/include/iterator:2459
+  !__lvalue_iter_move<_Ip> &&
+  __dereferenceable<_Ip>;
+
----------------
`__dereferenceable` is stronger than the spec, which only requires `*E` to be well-formed, not that it returns something referenceable.


================
Comment at: libcxx/include/iterator:2475
+  // ```
+  template<__unqualified_iter_move _Ip>
+  constexpr decltype(auto) operator()(_Ip&& __i) const
----------------
cjdb wrote:
> miscco wrote:
> > I am slightly concerned about the number of overloads that are introduced and the instantiation cost, which is quite high on clang right now due to lack of memoization of concepts.
> > 
> > I would rather put everything in a single `operator()` and then use `if constexpr` to return the right thing.
> > 
> > That would enable us to simplify the concept definitions, as we do not need to exclude the previous concepts  
> > which is quite high on clang right now due to lack of memoization of concepts.
> 
> How long ago did Clang disable memoisation?
> 
> > I would rather put everything in a single `operator()` and then use `if constexpr` to return the right thing.
> 
> I'm aware of this optimisation from GCC Concepts TS days, but I'd like to see evidence that Clang will get a significant performance boost from this too.
> It'll also mean that we can't have conditional noexcept, something I'm reluctant to give up.
Conditional noexcept is required for conformance since the wording uses expression-equivalent.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99873