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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83902