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

[David Blaikie via Phabricator via llvm-commits]

dblaikie 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);
+}
----------------
ychen wrote:
> ychen wrote:
> > dblaikie wrote:
> > > ychen wrote:
> > > > dblaikie wrote:
> > > > > gamesh411 wrote:
> > > > > > 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. 
> > > > > Writing const and non-const overloads of all of these seems a bit of a pain that I'd rather avoid, with pretty low-risk of downsides - and I'm generally pretty risk averse.
> > > > > 
> > > > > If a container does implement an rvalue overload for begin/end - I mean, what would that mean exactly? (if it provided only the rudimentary guarantee of "valid but unspecified state" - I could more reasonably imagine a container with && begin/end overloads that don't produce a "valid but unspecified state" but instead specifically produce move iterators & do so correctly when you call both begin and end)
> > > > > 
> > > > > So maybe we do need the std::forward - to cope with temporary containers, even if they aren't being moved-from? Anyone care to test that?
> > > > > 
> > > > > My test, at least:
> > > > > ```
> > > > > #include <utility>   
> > > > > void f1(const int&) = delete;  
> > > > > void f1(int&);  
> > > > > void f1(const float&);  
> > > > > void f1(float&) = delete;  
> > > > > template<typename T>  
> > > > > void t1(T &&t) {  
> > > > >   f1(t);  
> > > > > }  
> > > > > int main() {  
> > > > >   int i;  
> > > > >   t1(i);  // compiles
> > > > >   const float f = 3.0;
> > > > >   t1(f);  // compiles
> > > > >   t1(3.0f) // fails to compile without std::forward
> > > > > }
> > > > > ```
> > > > > 
> > > > > Pretty narrow-use-case, would only come up if you had non-member begin/end that took a const R&? We don't really have a lot of non-member begin/end anyway (probably none? nearly?) - but seems like a good thing to generalize over.
> > > > The benefit is to let API not take rvalue when it does not mean to. But in this case, this requires, as @gamesh411  said, duplicating the const and non-const L-value version for more than 10 methods. 
> > > > 
> > > > So the universal reference here is to reduce the number of overloads. So probably we should remove the `std::forwarding` along the stack to indicate that the rvalue does not matter.
> > > > 
> > > > It is also probably worthwhile to add a comment about this in general. 
> > > I think it should take rvalues, though, as in:
> > > ```
> > > llvm::move(build_container(), std::back_inserter(v));
> > > ```
> > > Or other such examples - in fact `move` seems like the poster child for algorithms you'd like to pass an rvalue to. Even if you don't have rvalue begin/end overloads.
> > > 
> > > The `std::forward` does seem potentially necessary to support that use case, again, even when there are no rvalue begin/end overloads - as in the f1/t1 example above, where a const ref and non-const ref overloads aren't handled correctly in the presence of a temporary passed through the template indirection. Even when none of the f1 overloads traffic in rvalue parameters themselves.
> > hmmm, interesting, yeah, for this to work, we need std::forward to keep the constness. 
> Could not resist to try it out: https://gcc.godbolt.org/z/T3hs5G (with std::forward removed along the stack)
> 
> rvalue should becomes lvalue down the stack so that adl_begin/end return non-const iterator to do the move. 
Not sure I follow - in any case, test cases (for this patch) would be good, to demonstrate what is/isn't needed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83902