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

Fri Jul 17 17:35:59 PDT 2020

gamesh411 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:
> ychen wrote:
> > dblaikie wrote:
> > > ychen wrote:
> > > > 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.
> > > Perhaps give it a go - but I believe that'd break use with const containers.
> > > 
> > > Essentially for the same reason this code fails to compile:
> > > ```
> > > template<typename T>
> > > void f1(T&);
> > > int main() {
> > >   f1(3); // No matching function for call to 'f1'  test.cpp:2:6: note: candidate function [with T = int] not viable: expects an l-value for 1st argument 
> > > } 
> > > ```
> > add a const l-value reference version should do it. std::begin has this version.
> > 
> > template<typename T>
> > void f1(const T&);
> What benefit would be gained by using overloads, rather than perfect forwarding?
This is painful... If you want to support both non-const and const iterators coming out of `adl_begin` *without perfect forwarding*, you are forced to do both overloads (the const-ref and the non-const-ref).
The added benefit would be, that you don't have to concern yourself with temporary containers. The downside would be that you have to maintain both overloads for every algorithm. The rationale behind not supporting pure rvalue (temporary) containers is that any iterators coming out of them would be invalidated by the time the algorithm returns, which is not practical. Const references would still allow this weird use-case, but then again there are algorithms which don't necessarily return an iterator (`for_each`). Those could be used with even a temporary.
I would be most definitely on the perfect forwarding side, if it could be guaranteed that forwarding to 2 functions is safe. This is safe if nowhere along the call stack is a move called on `Range`. if `adl_begin` *only ever* calls `begin` *and* begin will *never* be overloaded on the instance variable being rvalue (like `it_t begin() && {}` inside the container, and doing something fancy inside), *or* if a free function begin is chosen that doesn't move the value (a move could occur in this case even if the chosen overload takes the param by value). Those conditions are most likely to hold now and even in the future, but the problem is you are depending on the implementation to be correct with no way of ensuring it is on the library level. 

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83902