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

[Nathan James via Phabricator via llvm-commits]

njames93 marked 4 inline comments as done.
njames93 added inline comments.


================
Comment at: llvm/unittests/ADT/STLExtrasTest.cpp:582
+
+  // Ensure input container is same size, but its contents were moved out.
+  EXPECT_EQ(In.size(), ItemCount);
----------------
gamesh411 wrote:
> We could implement a dummy class here in the test code with an explicit move and copy ctors and assignment operators to not depend on the behaviour of std::string. That's because if small-string optimization is employed in the implementation of std::string, it is perfectly valid, and thus possible that the 'valid but unspecified' state of the moved-from string is not empty.
Thats a good point, this ought to do the job regardless of implementation specifics.


================
Comment at: llvm/unittests/ADT/STLExtrasTest.cpp:584
+  EXPECT_EQ(In.size(), ItemCount);
+  EXPECT_TRUE(llvm::all_of(In, [](const auto &Str) { return Str.empty(); }));
+
----------------
dblaikie wrote:
> Moved-from strings aren't guaranteed to be empty (they're in a "valid but unspecified" state - for small strings it's possible they still contain the original value) - probably using a specific type that counts moves/copies/destructions might help.
> 
> Range<Movable, Copyable> further up in this file provides that - but perhaps could use a refactor Maybe pull out most of the functionality (except the begin/end function) into a "LifetimeTracker" class - and then use that in this test?
I just created a moveable class, seemed simplest and safest way.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83902