[ADT] Make DenseMapBase::swap the public entry point

[kazutakahirata]

Without this patch, DenseMap::swap and SmallDenseMap::swap are
inconsistent because DenseMap::swap increments the epoch while
SmallDenseMap::swap does not.

This patch solves the inconsistency by making DenseMapBase::swap the
public entry point and renaming the existing swap to swapImpl.

To ease the review process, this patch does not move or group
functions according to access specifiers like private: and protected:.

[llvmbot]

@llvm/pr-subscribers-llvm-adt

Author: Kazu Hirata (kazutakahirata)

Changes

Without this patch, DenseMap::swap and SmallDenseMap::swap are
inconsistent because DenseMap::swap increments the epoch while
SmallDenseMap::swap does not.

This patch solves the inconsistency by making DenseMapBase::swap the
public entry point and renaming the existing swap to swapImpl.

To ease the review process, this patch does not move or group
functions according to access specifiers like private: and protected:.

---

Full diff: https://github.com/llvm/llvm-project/pull/167650.diff

1 Files Affected:

• (modified) llvm/include/llvm/ADT/DenseMap.h (+16-8)

diff --git a/llvm/include/llvm/ADT/DenseMap.h b/llvm/include/llvm/ADT/DenseMap.h
index 22ef7ed64451e..d5b13e7731550 100644
--- a/llvm/include/llvm/ADT/DenseMap.h
+++ b/llvm/include/llvm/ADT/DenseMap.h
@@ -360,6 +360,12 @@ class DenseMapBase : public DebugEpochBase {
     return getBuckets();
   }
 
+  void swap(DerivedT &RHS) {
+    this->incrementEpoch();
+    RHS.incrementEpoch();
+    derived().swapImpl(RHS);
+  }
+
 protected:
   DenseMapBase() = default;
 
@@ -736,7 +742,7 @@ class DenseMap : public DenseMapBase<DenseMap<KeyT, ValueT, KeyInfoT, BucketT>,
 
   DenseMap(DenseMap &&other) : BaseT() {
     init(0);
-    swap(other);
+    this->swap(other);
   }
 
   template <typename InputIt> DenseMap(const InputIt &I, const InputIt &E) {
@@ -756,15 +762,15 @@ class DenseMap : public DenseMapBase<DenseMap<KeyT, ValueT, KeyInfoT, BucketT>,
     deallocateBuckets();
   }
 
-  void swap(DenseMap &RHS) {
-    this->incrementEpoch();
-    RHS.incrementEpoch();
+private:
+  void swapImpl(DenseMap &RHS) {
     std::swap(Buckets, RHS.Buckets);
     std::swap(NumEntries, RHS.NumEntries);
     std::swap(NumTombstones, RHS.NumTombstones);
     std::swap(NumBuckets, RHS.NumBuckets);
   }
 
+public:
   DenseMap &operator=(const DenseMap &other) {
     if (&other != this)
       this->copyFrom(other);
@@ -775,7 +781,7 @@ class DenseMap : public DenseMapBase<DenseMap<KeyT, ValueT, KeyInfoT, BucketT>,
     this->destroyAll();
     deallocateBuckets();
     init(0);
-    swap(other);
+    this->swap(other);
     return *this;
   }
 
@@ -895,7 +901,7 @@ class SmallDenseMap
 
   SmallDenseMap(SmallDenseMap &&other) : BaseT() {
     init(0);
-    swap(other);
+    this->swap(other);
   }
 
   template <typename InputIt>
@@ -916,7 +922,8 @@ class SmallDenseMap
     deallocateBuckets();
   }
 
-  void swap(SmallDenseMap &RHS) {
+private:
+  void swapImpl(SmallDenseMap &RHS) {
     unsigned TmpNumEntries = RHS.NumEntries;
     RHS.NumEntries = NumEntries;
     NumEntries = TmpNumEntries;
@@ -987,6 +994,7 @@ class SmallDenseMap
     new (SmallSide.getLargeRep()) LargeRep(std::move(TmpRep));
   }
 
+public:
   SmallDenseMap &operator=(const SmallDenseMap &other) {
     if (&other != this)
       this->copyFrom(other);
@@ -997,7 +1005,7 @@ class SmallDenseMap
     this->destroyAll();
     deallocateBuckets();
     init(0);
-    swap(other);
+    this->swap(other);
     return *this;
   }
 

[kuhar]

Is the epoch number something we could test for?

[kazutakahirata]

Yes, in a somewhat limited manner. The epoch number is hiding in LLVM_ENABLE_ABI_BREAKING_CHECKS, so we could test it when the macro is on.

If turn it on ourselves in a unit test, then we might run into issues with other source files -- with two different definitions of DenseMap in a unit test executable.