[libcxx-commits] [PATCH] D129390: [lib++][ranges][NFC] Refactor `iterator_operations.h` to use tags.

[Hui via Phabricator via libcxx-commits]

huixie90 added inline comments.


================
Comment at: libcxx/include/__algorithm/iterator_operations.h:67
+  // Declaring the return type is necessary for the C++03 mode (which doesn't support placeholder return types).
+  static typename iterator_traits<typename remove_reference<_Iter>::type>::value_type&& __iter_move(_Iter&& __i) {
+    return std::move(*std::forward<_Iter>(__i));
----------------
I think you need to remove all the cv_ref instead of just the `reference`


================
Comment at: libcxx/include/__algorithm/iterator_operations.h:67
+  // Declaring the return type is necessary for the C++03 mode (which doesn't support placeholder return types).
+  static typename iterator_traits<typename remove_reference<_Iter>::type>::value_type&& __iter_move(_Iter&& __i) {
+    return std::move(*std::forward<_Iter>(__i));
----------------
huixie90 wrote:
> I think you need to remove all the cv_ref instead of just the `reference`
I thought I've commented somewhere I don't seem to find it anymore. Just be aware that the type of ` std::move(*std::forward<_Iter>(__i));` does not always match 
`typename iterator_traits<typename remove_reference<_Iter>::type>::value_type&&`. e.g. `vector<bool>::itertor`. But I think the code you've written is fine because most of the time the users do want the conversion (except for the new c++20 stuff which is not meant to be usable for classic algorithms)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129390