[PATCH] D83902: [ADT] Add a range-based version of std::move

[Yuanfang Chen via Phabricator via llvm-commits]

ychen added inline comments.


================
Comment at: llvm/include/llvm/ADT/STLExtras.h:1550
+OutputIt move(R &&Range, OutputIt Out) {
+  return std::move(adl_begin(Range), adl_end(Range), Out);
+}
----------------
gamesh411 wrote:
> njames93 wrote:
> > ychen wrote:
> > > Why std::forward is not used?
> > Forwarding the Range or the Output iterator. Forwarding the range to adl_begin and adl_end is not a good idea as if you pass an rvalue e.g. `llvm::move(std::move(vec), output)`, You're gonna have a bad time, I guess the output iterator could be forwarded, not sure how much would be gained from that.
> IMO, if `return std::move(adl_begin(std::forward<R>(Range)), adl_end(std::forward<R>(Range)), Out);` was meant, then forwarding to 2 calls is not really wise, as even if function argument evaluation is undefined but not overlapping (as from c++17 I think), one of the move-s will happen before the other, and the other move would be a use-after-move error.
> 
> Forwarding `Out` is out of the question as it is not a universal reference, and moving the result out would be a potentially unnecessary move (RVO with 2 stage overload resolution comes into mind), and nothing would be gained by explicitly moving the return value of the `std::move` algorithm.
> 
> Finally, iterators themselves are implemented with value semantics ingrained, designed to be cheap to copy.
> 
> All in all, I would only see merit in forwarding if Range was used exactly once, but even then perfect forwarding can interfere with overload resolution and type deduction, which is not something you want in such a generic piece of code. 
I meant forwarding `Range`. We don't have to call forwarding twice. We could call it once. The point is the `Range` is of universal reference type (which means `llvm::move(std::move(vec), output)` should be a valid case), there should a reason not to forwarding. This is more about the type-safety range than efficiency.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83902