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

[Orlando Cazalet-Hyams via Phabricator via llvm-commits]

Orlando marked 2 inline comments as done.
Orlando added inline comments.


================
Comment at: llvm/include/llvm/ADT/IntervalMap.h:1066-1069
+    if (RHS.branched())
+      rootBranch() = std::move(RHS.rootBranch());
+    else
+      rootLeaf() = std::move(RHS.rootLeaf());
----------------
dblaikie wrote:
> This leaves the leaves the `rootBranch`/`rootLeaf` in a moved-from state, what does that mean for the state of the object? 
> 
> What's the state of a moved-from `IntervalMap`? Is there some test coverage for that? I'd expect to need to change the `height`/`rootSize` for the moved-from object in some way?
> This leaves the leaves the rootBranch/rootLeaf in a moved-from state, what does that mean for the state of the object?

Yep you're right, my mistake. Setting `RHS.height = 0` stops the allocated data now owned by moved-into being deallocated by moved-from upon destruction, but that it doesn't stop the `leaf` destructor explicitly being called in the `IntervalMap` destructor.  In the case where moved-from (`RHS`) was using `branchData` instead of `leaf` the destructor of the wrong type is being called.

I think I've fixed this now!

> What's the state of a moved-from IntervalMap? Is there some test coverage for that?

I had intended for the test to cover this but it turns out that not enough intervals were added to trigger a change from `rootLeaf` to `rootBranch` (see `MoveAssignmentDst` in the unittests). This meant that no nodes were heap allocated, so asan didn't catch this mistake. Adding more intervals causes asan to find this issue.




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

https://reviews.llvm.org/D136242