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

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: llvm/include/llvm/ADT/IntervalMap.h:1052
+  // 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;
----------------
kparzysz wrote:
> dblaikie wrote:
> > 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.
> 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.
Fair enough - thanks for the context


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126401