[PATCH] D78425: [ValueLattice] Add move constructor

[Nikita Popov via Phabricator via llvm-commits]

nikic marked an inline comment as done.
nikic added inline comments.


================
Comment at: include/llvm/Analysis/ValueLattice.h:153
+    if (isConstantRange())
+      Range.~ConstantRange();
+
----------------
fhahn wrote:
> nikic wrote:
> > I went for a slightly simpler implementation than the copy assignment operator here by always destroying and constructing, rather than trying to do an assignment if we go from one range to another range. That makes (theoretical, but probably not practical) sense for copy assignment, but not for move assignment.
> Yeah that should be fine. The main reason was to avoid constructing a range unnecessarily, but as the vast majority of ranges has tbitwidths <= 64 that should not matter in practice. Best to keep the assignment & move operators in sync in that respect.
After doing that change, I think it makes sense to implement the assignment operators in terms of destruct + construct, rather than implementing the constructors in terms of default construct and assign. The copy/move constructors are the more primitive operations.

I wasn't completely sure whether writing `new (this)` is safe, but it looks like there's already parts of LLVM using exactly this pattern, such as `ImplicitConversionSequence`.


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

https://reviews.llvm.org/D78425