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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.

I disagree with the choice to use `if constexpr` over overload resolution for compile-time performance reasons. I think we should continue to strive to keep the code simple. No offense, but the latest revision of this patch with `if constexpr` is *significantly* more complicated than a naive implementation like this (which is similar to what Chris suggested originally, but a bit simpler IMO):

  namespace ranges::__iter_move {
  void iter_move();
  
  template<class _Ip>
  concept __unqualified_iter_move = requires(_Ip&& __i) {
      iter_move(_VSTD::forward<_Ip>(__i));
  };
  
  // [iterator.cust.move]/1
  // The name ranges​::​iter_­move denotes a customization point object. 
  // The expression ranges​::​iter_­move(E) for a subexpression E is 
  // expression-equivalent to:
  struct __fn {
    // [iterator.cust.move]/1.1
    // iter_­move(E), if E has class or enumeration type and iter_­move(E) is a 
    // well-formed expression when treated as an unevaluated operand, [...]
    template<class _Ip>
      requires __class_or_enum<remove_cvref_t<_Ip>> && __unqualified_iter_move<_Ip>
    constexpr decltype(auto) operator()(_Ip&& __i) const 
      noexcept(iter_move(_VSTD::forward<_Ip>(__i))) 
    {
      return iter_move(_VSTD::forward<_Ip>(__i));
    }
  
    // [iterator.cust.move]/1.2
    // Otherwise, if the expression *E is well-formed:
    //  1.2.1 if *E is an lvalue, std​::​move(*E);
    //  1.2.2 otherwise, *E.
    template<class _Ip>
      requires !(__class_or_enum<remove_cvref_t<_Ip>> && __unqualified_iter_move<_Ip>) &&
      requires(_Ip&& __i) { *_VSTD::forward<_Ip>(__i); }
    constexpr decltype(auto) operator()(_Ip&& __i) const 
      noexcept(*_VSTD::forward<_Ip>(__i))
    {
      if constexpr (is_lvalue_reference_v<decltype(*_VSTD::forward<_Ip>(__i))>) {
        return _VSTD::move(*_VSTD::forward<_Ip>(__i));
      } else {
        return *_VSTD::forward<_Ip>(__i);
      }
    }
  
    // [iterator.cust.move]/1.3
    // Otherwise, ranges​::​iter_­move(E) is ill-formed.
  };
  } // namespace ranges::__iter_move

Bending the implementation backwards to improve compilation times when the problem is actually at the language level (overload resolution being so expensive) and at the specification level (they could have decided to go for much simpler rules with a lot less expensive SFINAE) is not the right way to go. If we want to improve the compile-times of Ranges in libc++, I'm all for finding ways to do that, but not by making our implementation significantly more complex and working around the language. Also, complexifying the implementation is not only a matter of "oh but it's hard to read". It's primarily about shipping a correct implementation, which I think is more important than shipping one that compiles fast.

Don't get me wrong, I spent like 3 years of my life trying to improve the compilation times in the C++ ecosystem by doing things like Boost.Hana and http://metaben.ch. I'm very sensitive to that problem. I just think that throwing `if constexpr` at the problem here is too little too late.



================
Comment at: libcxx/include/concepts:250-253
+concept __class_or_enum =
+  is_class_v<remove_cvref_t<_Tp>> ||
+  is_union_v<remove_cvref_t<_Tp>> ||
+  is_enum_v<remove_cvref_t<_Tp>>;
----------------
I really think the `remove_cvref_t` doesn't belong here. It belongs to the callers of this concept.

Even though this concept is just a private helper, I think we should strive to make it meaningful on its own.


================
Comment at: libcxx/include/iterator:48
+  }
+using iter_rvalue_reference_t
+  = decltype(ranges::iter_move(declval<T&>()));
----------------
Missing `// Since C++20`


================
Comment at: libcxx/include/iterator:2425
 
+#if !defined(_LIBCPP_HAS_NO_RANGES)
+
----------------
Would it make sense to add this in `__ranges/iter_move.h`?


================
Comment at: libcxx/include/iterator:2447
+concept __rvalue_iter_move =
+  !__unqualified_iter_move<_Ip> &&
+  !__lvalue_iter_move<_Ip> &&
----------------
This is redundant since it's already included in the check for `!__lvalue_iter_move<_Ip>`.


================
Comment at: libcxx/include/iterator:2453
+
+template<class> inline constexpr bool __always_false = false;
+
----------------
Not used anywhere.


================
Comment at: libcxx/include/iterator:2503
+
+    // Otherwise, `ranges​::​iter_­move(E)` is ill-formed.
+  }
----------------
More strange `!`s


================
Comment at: libcxx/test/std/iterators/iterator.requirements/iterator.cust/iterator.cust.move/iter_move.pass.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
Can you please add a test with an iterator that can't be `iter_move`d to confirm that `ranges::iter_move(E)` SFINAEs away? With the `if constexpr` implementation, I don't think that's the case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99873