[PATCH] D78425: [ValueLattice] Add move constructor

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: include/llvm/Analysis/ValueLattice.h:129
-        !Other.isNotConstant())
-      ConstVal = nullptr;
-
----------------
nikic wrote:
> Happy to commit this part separately: This nulling of ConstVal looks unnecessary to me. `ConstVal` is not an owned pointer, so this doesn't free it. If we change to a state that does not the ConstVal, then the value simply does not matter and we can leave it alone.
It would be great if you could commit that separately.


================
Comment at: include/llvm/Analysis/ValueLattice.h:153
+    if (isConstantRange())
+      Range.~ConstantRange();
+
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78425