[PATCH] D136242: [IntervalMap] Add move and copy ctors and assignment operators

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: llvm/include/llvm/ADT/IntervalMap.h:1059
+
+  IntervalMap(IntervalMap &&RHS) { *this = std::move(RHS); }
+  IntervalMap &operator=(IntervalMap &&RHS) {
----------------
Orlando wrote:
> Hmmm. Thinking about it, should I construct `data` in the move constructor like in the explicit constructor ? Since `IntervalMap`'s dtor explicitly calls `data`'s dtor (`rootLeaf().~RootLeaf()`). Otherwise the RHS map containing the swapped `data` will try to call the `RootLeaf` dtor explicitly on an object that hasn't been constructed, which... feels like UB territory to me?
Yep, sounds problematic (any chance you can test under sanitizers and see if they detect the problem? Otherwise I guess manually stepping through with a debugger might verify the behavior). Could use a forwarding ctor, perhaps?

Same would be true of the copy constructor too? Since that calls the assignment operator which calls clear, which presumably tries to destroy some things. Hmm, maybe it skates through OK - because it 's not-branched and just sets the size to zero?

(I suspect there's some other "technical" UB around here - like calling `rootLeaf` to /then/ construct it in the Allocator ctor is probably UB because we've bound a reference to an object before we ran the ctor on it - but that's pre-existing. Probably the right thing to do is to get a void pointer to the storage, call the ctor on that)


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

https://reviews.llvm.org/D136242