[ADT] Explicitly delete copy/move constructors and operator= in IntervalMap

[kparzysz]

Summary

The default ones may do the wrong thing, for example perform a shallow copy instead of a deep copy. Disable them so they don't get accidentally used.

[dblaikie]

Sounds good - maybe add some static_asserts in the unit test to validate that these operations are appropriately deleted? & could you make the comment more definitive - "Delete these operations as the defaults would be shallow/cause incorrect shared state. Implement these if/when needed/worthwhile to do so." or something like that.

[dblaikie]

llvm/include/llvm/ADT/IntervalMap.h

1052

This isn't quite accurate - the "std::pair" situation came up only on FreeBSD when std::pair of trivially copyable types wasn't itself trivially copyable. The general statement is true for any case where the value type is not trivially copyable, but maybe that's not worth mentioning in a comment here? Enough to say that these operations wouldn't be correct (regardless of whether the incorrectness/failure is a compile time or runtime issue?)

So I'd probably just remove this two line comment, keep the one above.

[kparzysz]

llvm/include/llvm/ADT/IntervalMap.h

1052

I mentioned it because the deletions of operator= here don't actually do anything, and the static asserts in the test will succeed regardless of whether they are present or not.
The reason why RootLeaf has a non-trivial copy operator is because it contains an array of std::pair, which is not trivially copy-assignable (including on Linux, so it isn't specific to FreeBSD). In particular, std::is_trivially_copy_assignable<std::pair<int, int>>::value is false. I mentioned std::pair explicitly to point to it as the cause, because it isn't always obvious out why some type has some property.

[dblaikie]

llvm/include/llvm/ADT/IntervalMap.h

1052

Fair enough - thanks for the context

llvm/include/llvm/ADT/IntervalMap.h

void visitNodes(void (IntervalMap::*f)(IntervalMapImpl::NodeRef,
                   unsigned Level));
   void deleteNode(IntervalMapImpl::NodeRef Node, unsigned Level);
 
 public:
   explicit IntervalMap(Allocator &a) : height(0), rootSize(0), allocator(a) {
     new(&rootLeaf()) RootLeaf();
   }
 
+  // The default copy/move constructors and assignment operators would perform
+  // a shallow copy, leading to an incorrect internal state. To prevent
+  // accidental use, explicitly delete these operators.
+  // If necessary, implement them to perform a deep copy.
+  IntervalMap(const IntervalMap &Other) = delete;
+  IntervalMap(IntervalMap &&Other) = delete;
+  // Note: these are already implicitly deleted, because RootLeaf (union
+  // member) has a non-trivial assignment operator (because of std::pair).
+  IntervalMap &operator=(const IntervalMap &Other) = delete;
+  IntervalMap &operator=(IntervalMap &&Other) = delete;
+
   ~IntervalMap() {
     clear();
     rootLeaf().~RootLeaf();
   }
 
   /// empty -  Return true when no intervals are mapped.
   bool empty() const {
     return rootSize == 0;

llvm/unittests/ADT/IntervalMapTest.cpp

 //===---- ADT/IntervalMapTest.cpp - IntervalMap unit tests ------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/ADT/IntervalMap.h"
 #include "gtest/gtest.h"
+#include <type_traits>
 
 using namespace llvm;
 
 namespace {
 
 typedef IntervalMap<unsigned, unsigned, 4> UUMap;
 typedef IntervalMap<unsigned, unsigned, 4,
                     IntervalMapHalfOpenInfo<unsigned>> UUHalfOpenMap;
 
+static_assert(!std::is_copy_constructible<UUMap>::value,
+              "IntervalMap copy constructor should be deleted");
+static_assert(!std::is_move_constructible<UUMap>::value,
+              "IntervalMap move constructor should be deleted");
+static_assert(!std::is_copy_assignable<UUMap>::value,
+              "IntervalMap copy assignment should be deleted");
+static_assert(!std::is_move_assignable<UUMap>::value,
+              "IntervalMap move assignment should be deleted");
+
 // Empty map tests
 TEST(IntervalMapTest, EmptyMap) {
   UUMap::Allocator allocator;
   UUMap map(allocator);
   EXPECT_TRUE(map.empty());
 
   // Lookup on empty map.
   EXPECT_EQ(0u, map.lookup(0));