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

Thu Jul 23 19:25:16 PDT 2020

dblaikie accepted this revision.
dblaikie added a comment.

Yeah, please commit this as-is, without the std::forward calls, and including the test cases you've provided that help exercise/demonstrate the necessity.

================
Comment at: llvm/unittests/ADT/STLExtrasTest.cpp:613
+
+  llvm::move(std::move(V2), std::back_inserter(V3));
+
----------------
gamesh411 wrote:
> ychen wrote:
> > njames93 wrote:
> > > When I put std::forward on the range, this call failed to compile as the copy constructor of `Foo` has been deleted. So for some reason when the container is forwarded to `adl_begin` and `adl_end`, it results in trying to create a copy of the container first.
> > > 
> > > This call though doesn't actually move `V2`, its contents have been moved, but the items are still in there (`size() == ItemCount`).
> > > That one has me a little perplexed. Should say nothing strange showed up with ASAN even when I changed to using heap allocation instead of the SmallVector inline storage.
> > Without std::forward in llvm::move, std::begin/end in adl_begin/end would get a lvalue(yes, the type are still rvalue ref), so non-const version of std::begin/end is called, so elements could be moved. 
> > 
> > If std::forward is in llvm::move, it would be perfect forwarded to std::begin/end, trigger its const version which return const iterator that could not be moved.
> > 
> > All in all, the universal reference here is confusing, and it is not meant for perfect forwarding, just a shortcut for overloading const and non-const reference of parameters.
> Good point! I have also been bitten by std::move algorithm falling back silently to copy-ing if somehow a const-iterator is given to it (and the copy ctor of the underlying element is not deleted). I agree that the main reason for universal ref here is to avoid overloading! :+1:
I don't think it's trying to create a copy of the container, so far as I can understand from the error. Here's the error I get:
```
/usr/local/google/home/blaikie/dev/llvm/src/llvm/include/llvm/ADT/SmallVector.h:249:33: error: call to deleted constructor of 'Foo'
    ::new ((void*) this->end()) T(Elt);
                                ^ ~~~
/usr/local/google/home/blaikie/install/bin/../lib/gcc/x86_64-pc-linux-gnu/10.0.0/../../../../include/c++/10.0.0/bits/stl_iterator.h:532:13: note: in instantiation of member function 'llvm::SmallVectorTemplateBase<Foo, false>::push_back' requested here
        container->push_back(__value);
                   ^
/usr/local/google/home/blaikie/install/bin/../lib/gcc/x86_64-pc-linux-gnu/10.0.0/../../../../include/c++/10.0.0/bits/stl_algobase.h:426:18: note: in instantiation of member function 'std::back_insert_iterator<llvm::SmallVector<Foo, 4>>::operator=' requested here
              *__result = std::move(*__first);
                        ^
/usr/local/google/home/blaikie/install/bin/../lib/gcc/x86_64-pc-linux-gnu/10.0.0/../../../../include/c++/10.0.0/bits/stl_algobase.h:499:22: note: in instantiation of function template specialization 'std::__copy_move<true, false, std::random_access_iterator_tag>::__copy_m<const Foo *, std::back_insert_iterator<llvm::SmallVector<Foo, 4>>>' requested here
                              _Category>::__copy_m(__first, __last, __result);
                                          ^
/usr/local/google/home/blaikie/install/bin/../lib/gcc/x86_64-pc-linux-gnu/10.0.0/../../../../include/c++/10.0.0/bits/stl_algobase.h:533:19: note: in instantiation of function template specialization 'std::__copy_move_a2<true, const Foo *, std::back_insert_iterator<llvm::SmallVector<Foo, 4>>>' requested here
    { return std::__copy_move_a2<_IsMove>(__first, __last, __result); }
                  ^
/usr/local/google/home/blaikie/install/bin/../lib/gcc/x86_64-pc-linux-gnu/10.0.0/../../../../include/c++/10.0.0/bits/stl_algobase.h:541:8: note: in instantiation of function template specialization 'std::__copy_move_a1<true, const Foo *, std::back_insert_iterator<llvm::SmallVector<Foo, 4>>>' requested here
                std::__copy_move_a1<_IsMove>(std::__niter_base(__first),
                     ^
/usr/local/google/home/blaikie/install/bin/../lib/gcc/x86_64-pc-linux-gnu/10.0.0/../../../../include/c++/10.0.0/bits/stl_algobase.h:628:19: note: in instantiation of function template specialization 'std::__copy_move_a<true, const Foo *, std::back_insert_iterator<llvm::SmallVector<Foo, 4>>>' requested here
      return std::__copy_move_a<true>(std::__miter_base(__first),
                  ^
/usr/local/google/home/blaikie/dev/llvm/src/llvm/include/llvm/ADT/STLExtras.h:1542:15: note: in instantiation of function template specialization 'std::move<const Foo *, std::back_insert_iterator<llvm::SmallVector<Foo, 4>>>' requested here 
  return std::move(adl_begin(std::forward<R>(Range)), adl_end(std::forward<R>(Range)), Out);
              ^
/usr/local/google/home/blaikie/dev/llvm/src/llvm/unittests/ADT/STLExtrasTest.cpp:613:9: note: in instantiation of function template specialization 'llvm::move<llvm::SmallVector<Foo, 4>, std::back_insert_iterator<llvm::SmallVector<Foo, 4>>>' requested here
  llvm::move(std::move(V2), std::back_inserter(V3));
        ^
/usr/local/google/home/blaikie/dev/llvm/src/llvm/unittests/ADT/STLExtrasTest.cpp:578:5: note: 'Foo' has been explicitly marked deleted here
    Foo(const Foo &) = delete;
    ^
```

I think the issue is that adl_begin(std::forward<R>(Container)) is giving an rvalue to adl_begin, which goes down, etc, etc, then std::begin which is const ref/non-const-ref overloaded ends up calling the const ref version.

On that basis, perhaps not using forward on calls to adl_begin/adl_end might actually be the right thing to do - means rvalues turn into lvalues (which they are, in that context - it was an rvalue to the original function, but an lvalue when asking for its begin/end).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83902