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

[Endre Fülöp via Phabricator via llvm-commits]

gamesh411 accepted this revision.
gamesh411 marked an inline comment as done.
gamesh411 added a comment.
This revision is now accepted and ready to land.

LGTM!



================
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);
+}
----------------
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. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83902