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

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

Orlando added a comment.

Thanks @dblaikie. I was about to land this and noticed some potential UB - I have put a question inline.



================
Comment at: llvm/include/llvm/ADT/IntervalMap.h:1044
 public:
-  explicit IntervalMap(Allocator &a) : height(0), rootSize(0), allocator(a) {
+  explicit IntervalMap(Allocator &a) : height(0), rootSize(0), allocator(&a) {
     assert((uintptr_t(&data) & (alignof(RootLeaf) - 1)) == 0 &&
----------------
dblaikie wrote:
> could drop the `height` and `rootSize` initializations now that they're initialized at the member declaration
Good point, will do.


================
Comment at: llvm/include/llvm/ADT/IntervalMap.h:1059
+
+  IntervalMap(IntervalMap &&RHS) { *this = std::move(RHS); }
+  IntervalMap &operator=(IntervalMap &&RHS) {
----------------
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?


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

https://reviews.llvm.org/D136242