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

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

gamesh411 added inline comments.


================
Comment at: llvm/unittests/ADT/STLExtrasTest.cpp:613
+
+  llvm::move(std::move(V2), std::back_inserter(V3));
+
----------------
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:


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83902