[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);
+}
----------------
dblaikie wrote:
> gamesh411 wrote:
> > ychen wrote:
> > > gamesh411 wrote:
> > > > ychen wrote:
> > > > > ychen wrote:
> > > > > > 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.
> > > > > Actually, calling std::forward twice is a little bit confusing but not wrong.
> > > > I am really no expert or authority, but you could be right if forwarding twice would somehow guarantee that both rvalue references refer to something that is 'safe to use' so to speak. It would go against my intuition, but again you might be right. However, even if this is the case, it should be done in every algorithm (like in `llvm::copy` above), and that alone would warrant that this change of applying perfect forwarding to all algorithms a separate patch.
> > > I wouldn't be 100% sure, but this is my intuition. I just realized that @dblaikie  authored many other similar ones as you mentioned. Probably he got better idea.
> > I would be really interested in a throughout explanation as well :) 
> Not a fantastically better idea, unfortunately - I've had similar thoughts to most sides of this debate.
> 
> I don't know of any container that offers, for instance, moving iterators when you retrieve begin/end from the range - so it's "optimistic" at best to have those std::forward calls in for some such hypothetical situation.
Digging a little bit more. I think `adl_detail::adl_begin(ContainerTy &&container)` should be `adl_detail::adl_begin(ContainerTy &container)` since std::begin does not have a universal reference version and that makes sense. With this, many other clients using adl_begin()/adl_end() need not use universal reference type.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83902