[ADT] refactor MoveOnly type in ADT unittest #94421

c8ef · 2024-06-05T02:35:57Z

context: #94151 (review)

llvmbot · 2024-06-05T02:36:32Z

@llvm/pr-subscribers-llvm-adt

Author: None (c8ef)

Changes

context: #94151 (review)

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

7 Files Affected:

(modified) llvm/unittests/ADT/CMakeLists.txt (+1-1)
(added) llvm/unittests/ADT/CountCopyAndMove.cpp (+17)
(added) llvm/unittests/ADT/CountCopyAndMove.h (+57)
(modified) llvm/unittests/ADT/DenseMapTest.cpp (+17-41)
(removed) llvm/unittests/ADT/MoveOnly.cpp (-15)
(removed) llvm/unittests/ADT/MoveOnly.h (-42)
(modified) llvm/unittests/ADT/STLForwardCompatTest.cpp (+27-27)

diff --git a/llvm/unittests/ADT/CMakeLists.txt b/llvm/unittests/ADT/CMakeLists.txt
index 17c5c9d1c59ce..85c140e63fecd 100644
--- a/llvm/unittests/ADT/CMakeLists.txt
+++ b/llvm/unittests/ADT/CMakeLists.txt
@@ -18,6 +18,7 @@ add_llvm_unittest(ADTTests
   CoalescingBitVectorTest.cpp
   CombinationGeneratorTest.cpp
   ConcurrentHashtableTest.cpp
+  CountCopyAndMove.cpp
   DAGDeltaAlgorithmTest.cpp
   DeltaAlgorithmTest.cpp
   DenseMapTest.cpp
@@ -52,7 +53,6 @@ add_llvm_unittest(ADTTests
   LazyAtomicPointerTest.cpp
   MappedIteratorTest.cpp
   MapVectorTest.cpp
-  MoveOnly.cpp
   PackedVectorTest.cpp
   PagedVectorTest.cpp
   PointerEmbeddedIntTest.cpp
diff --git a/llvm/unittests/ADT/CountCopyAndMove.cpp b/llvm/unittests/ADT/CountCopyAndMove.cpp
new file mode 100644
index 0000000000000..fe1e2f4a5b89f
--- /dev/null
+++ b/llvm/unittests/ADT/CountCopyAndMove.cpp
@@ -0,0 +1,17 @@
+//===- llvm/unittest/ADT/CountCopyAndMove.cpp - Optional unit tests -------===//
+//
+// 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 "CountCopyAndMove.h"
+
+using namespace llvm;
+
+int CountCopyAndMove::CopyConstructions = 0;
+int CountCopyAndMove::CopyAssignments = 0;
+int CountCopyAndMove::MoveConstructions = 0;
+int CountCopyAndMove::MoveAssignments = 0;
+int CountCopyAndMove::Destructions = 0;
diff --git a/llvm/unittests/ADT/CountCopyAndMove.h b/llvm/unittests/ADT/CountCopyAndMove.h
new file mode 100644
index 0000000000000..126054427b81a
--- /dev/null
+++ b/llvm/unittests/ADT/CountCopyAndMove.h
@@ -0,0 +1,57 @@
+//===- llvm/unittest/ADT/CountCopyAndMove.h - Optional unit tests ---------===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_UNITTESTS_ADT_COUNTCOPYANDMOVE_H
+#define LLVM_UNITTESTS_ADT_COUNTCOPYANDMOVE_H
+
+namespace llvm {
+
+struct CountCopyAndMove {
+  static int CopyConstructions;
+  static int CopyAssignments;
+  static int MoveConstructions;
+  static int MoveAssignments;
+  static int Destructions;
+  int val;
+
+  CountCopyAndMove() = default;
+  explicit CountCopyAndMove(int val) : val(val) {}
+  CountCopyAndMove(const CountCopyAndMove &other) : val(other.val) {
+    ++CopyConstructions;
+  }
+  CountCopyAndMove &operator=(const CountCopyAndMove &other) {
+    val = other.val;
+    ++CopyAssignments;
+    return *this;
+  }
+  CountCopyAndMove(CountCopyAndMove &&other) : val(other.val) {
+    ++MoveConstructions;
+  }
+  CountCopyAndMove &operator=(CountCopyAndMove &&other) {
+    val = other.val;
+    ++MoveAssignments;
+    return *this;
+  }
+  ~CountCopyAndMove() { ++Destructions; }
+
+  static void ResetCounts() {
+    CopyConstructions = 0;
+    CopyAssignments = 0;
+    MoveConstructions = 0;
+    MoveAssignments = 0;
+    Destructions = 0;
+  }
+
+  static int TotalCopies() { return CopyConstructions + CopyAssignments; }
+
+  static int TotalMoves() { return MoveConstructions + MoveAssignments; }
+};
+
+} // end namespace llvm
+
+#endif // LLVM_UNITTESTS_ADT_COUNTCOPYANDMOVE_H
diff --git a/llvm/unittests/ADT/DenseMapTest.cpp b/llvm/unittests/ADT/DenseMapTest.cpp
index cc3244528f27e..09f9a57647709 100644
--- a/llvm/unittests/ADT/DenseMapTest.cpp
+++ b/llvm/unittests/ADT/DenseMapTest.cpp
@@ -7,6 +7,7 @@
 //===----------------------------------------------------------------------===//
 
 #include "llvm/ADT/DenseMap.h"
+#include "CountCopyAndMove.h"
 #include "llvm/ADT/DenseMapInfo.h"
 #include "llvm/ADT/DenseMapInfoVariant.h"
 #include "llvm/ADT/StringRef.h"
@@ -346,29 +347,6 @@ TYPED_TEST(DenseMapTest, ConstIteratorTest) {
   EXPECT_TRUE(cit == cit2);
 }
 
-namespace {
-// Simple class that counts how many moves and copy happens when growing a map
-struct CountCopyAndMove {
-  static int Move;
-  static int Copy;
-  CountCopyAndMove() {}
-
-  CountCopyAndMove(const CountCopyAndMove &) { Copy++; }
-  CountCopyAndMove &operator=(const CountCopyAndMove &) {
-    Copy++;
-    return *this;
-  }
-  CountCopyAndMove(CountCopyAndMove &&) { Move++; }
-  CountCopyAndMove &operator=(const CountCopyAndMove &&) {
-    Move++;
-    return *this;
-  }
-};
-int CountCopyAndMove::Copy = 0;
-int CountCopyAndMove::Move = 0;
-
-} // anonymous namespace
-
 // Test initializer list construction.
 TEST(DenseMapCustomTest, InitializerList) {
   DenseMap<int, int> M({{0, 0}, {0, 1}, {1, 2}});
@@ -401,8 +379,8 @@ TEST(DenseMapCustomTest, DefaultMinReservedSizeTest) {
   // Will allocate 64 buckets
   Map.reserve(1);
   unsigned MemorySize = Map.getMemorySize();
-  CountCopyAndMove::Copy = 0;
-  CountCopyAndMove::Move = 0;
+  CountCopyAndMove::ResetCounts();
+
   for (int i = 0; i < ExpectedMaxInitialEntries; ++i)
     Map.insert(std::pair<int, CountCopyAndMove>(std::piecewise_construct,
                                                 std::forward_as_tuple(i),
@@ -410,9 +388,9 @@ TEST(DenseMapCustomTest, DefaultMinReservedSizeTest) {
   // Check that we didn't grow
   EXPECT_EQ(MemorySize, Map.getMemorySize());
   // Check that move was called the expected number of times
-  EXPECT_EQ(ExpectedMaxInitialEntries, CountCopyAndMove::Move);
+  EXPECT_EQ(ExpectedMaxInitialEntries, CountCopyAndMove::TotalMoves());
   // Check that no copy occurred
-  EXPECT_EQ(0, CountCopyAndMove::Copy);
+  EXPECT_EQ(0, CountCopyAndMove::TotalCopies());
 
   // Adding one extra element should grow the map
   Map.insert(std::pair<int, CountCopyAndMove>(
@@ -425,7 +403,7 @@ TEST(DenseMapCustomTest, DefaultMinReservedSizeTest) {
   //  This relies on move-construction elision, and cannot be reliably tested.
   //   EXPECT_EQ(ExpectedMaxInitialEntries + 2, CountCopyAndMove::Move);
   // Check that no copy occurred
-  EXPECT_EQ(0, CountCopyAndMove::Copy);
+  EXPECT_EQ(0, CountCopyAndMove::TotalCopies());
 }
 
 // Make sure creating the map with an initial size of N actually gives us enough
@@ -439,8 +417,8 @@ TEST(DenseMapCustomTest, InitialSizeTest) {
   for (auto Size : {1, 2, 48, 66}) {
     DenseMap<int, CountCopyAndMove> Map(Size);
     unsigned MemorySize = Map.getMemorySize();
-    CountCopyAndMove::Copy = 0;
-    CountCopyAndMove::Move = 0;
+    CountCopyAndMove::ResetCounts();
+
     for (int i = 0; i < Size; ++i)
       Map.insert(std::pair<int, CountCopyAndMove>(std::piecewise_construct,
                                                   std::forward_as_tuple(i),
@@ -448,9 +426,9 @@ TEST(DenseMapCustomTest, InitialSizeTest) {
     // Check that we didn't grow
     EXPECT_EQ(MemorySize, Map.getMemorySize());
     // Check that move was called the expected number of times
-    EXPECT_EQ(Size, CountCopyAndMove::Move);
+    EXPECT_EQ(Size, CountCopyAndMove::TotalMoves());
     // Check that no copy occurred
-    EXPECT_EQ(0, CountCopyAndMove::Copy);
+    EXPECT_EQ(0, CountCopyAndMove::TotalCopies());
   }
 }
 
@@ -461,15 +439,14 @@ TEST(DenseMapCustomTest, InitFromIterator) {
   const int Count = 65;
   Values.reserve(Count);
   for (int i = 0; i < Count; i++)
-    Values.emplace_back(i, CountCopyAndMove());
+    Values.emplace_back(i, CountCopyAndMove(i));
 
-  CountCopyAndMove::Move = 0;
-  CountCopyAndMove::Copy = 0;
+  CountCopyAndMove::ResetCounts();
   DenseMap<int, CountCopyAndMove> Map(Values.begin(), Values.end());
   // Check that no move occurred
-  EXPECT_EQ(0, CountCopyAndMove::Move);
+  EXPECT_EQ(0, CountCopyAndMove::TotalMoves());
   // Check that copy was called the expected number of times
-  EXPECT_EQ(Count, CountCopyAndMove::Copy);
+  EXPECT_EQ(Count, CountCopyAndMove::TotalCopies());
 }
 
 // Make sure reserve actually gives us enough buckets to insert N items
@@ -484,8 +461,7 @@ TEST(DenseMapCustomTest, ReserveTest) {
     DenseMap<int, CountCopyAndMove> Map;
     Map.reserve(Size);
     unsigned MemorySize = Map.getMemorySize();
-    CountCopyAndMove::Copy = 0;
-    CountCopyAndMove::Move = 0;
+    CountCopyAndMove::ResetCounts();
     for (int i = 0; i < Size; ++i)
       Map.insert(std::pair<int, CountCopyAndMove>(std::piecewise_construct,
                                                   std::forward_as_tuple(i),
@@ -493,9 +469,9 @@ TEST(DenseMapCustomTest, ReserveTest) {
     // Check that we didn't grow
     EXPECT_EQ(MemorySize, Map.getMemorySize());
     // Check that move was called the expected number of times
-    EXPECT_EQ(Size, CountCopyAndMove::Move);
+    EXPECT_EQ(Size, CountCopyAndMove::TotalMoves());
     // Check that no copy occurred
-    EXPECT_EQ(0, CountCopyAndMove::Copy);
+    EXPECT_EQ(0, CountCopyAndMove::TotalCopies());
   }
 }
 
diff --git a/llvm/unittests/ADT/MoveOnly.cpp b/llvm/unittests/ADT/MoveOnly.cpp
deleted file mode 100644
index 541903bbe9467..0000000000000
--- a/llvm/unittests/ADT/MoveOnly.cpp
+++ /dev/null
@@ -1,15 +0,0 @@
-//===- llvm/unittest/ADT/MoveOnly.cpp - Optional unit tests ---------------===//
-//
-// 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 "MoveOnly.h"
-
-using namespace llvm;
-
-unsigned MoveOnly::MoveConstructions = 0;
-unsigned MoveOnly::Destructions = 0;
-unsigned MoveOnly::MoveAssignments = 0;
diff --git a/llvm/unittests/ADT/MoveOnly.h b/llvm/unittests/ADT/MoveOnly.h
deleted file mode 100644
index ad64deae771ba..0000000000000
--- a/llvm/unittests/ADT/MoveOnly.h
+++ /dev/null
@@ -1,42 +0,0 @@
-//===- llvm/unittest/ADT/MoveOnly.h - Optional unit tests -----------------===//
-//
-// 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
-//
-//===----------------------------------------------------------------------===//
-
-#ifndef LLVM_UNITTESTS_ADT_MOVEONLY_H
-#define LLVM_UNITTESTS_ADT_MOVEONLY_H
-
-namespace llvm {
-
-struct MoveOnly {
-  static unsigned MoveConstructions;
-  static unsigned Destructions;
-  static unsigned MoveAssignments;
-  int val;
-  explicit MoveOnly(int val) : val(val) {
-  }
-  MoveOnly(MoveOnly&& other) {
-    val = other.val;
-    ++MoveConstructions;
-  }
-  MoveOnly &operator=(MoveOnly&& other) {
-    val = other.val;
-    ++MoveAssignments;
-    return *this;
-  }
-  ~MoveOnly() {
-    ++Destructions;
-  }
-  static void ResetCounts() {
-    MoveConstructions = 0;
-    Destructions = 0;
-    MoveAssignments = 0;
-  }
-};
-
-}  // end namespace llvm
-
-#endif // LLVM_UNITTESTS_ADT_MOVEONLY_H
diff --git a/llvm/unittests/ADT/STLForwardCompatTest.cpp b/llvm/unittests/ADT/STLForwardCompatTest.cpp
index b0c95d09ba2c6..7300b4883a173 100644
--- a/llvm/unittests/ADT/STLForwardCompatTest.cpp
+++ b/llvm/unittests/ADT/STLForwardCompatTest.cpp
@@ -7,7 +7,7 @@
 //===----------------------------------------------------------------------===//
 
 #include "llvm/ADT/STLForwardCompat.h"
-#include "MoveOnly.h"
+#include "CountCopyAndMove.h"
 #include "gtest/gtest.h"
 
 namespace {
@@ -58,27 +58,27 @@ TEST(TransformTest, TransformStd) {
 }
 
 TEST(TransformTest, MoveTransformStd) {
-  using llvm::MoveOnly;
+  using llvm::CountCopyAndMove;
 
-  std::optional<MoveOnly> A;
+  std::optional<CountCopyAndMove> A;
 
-  MoveOnly::ResetCounts();
+  CountCopyAndMove::ResetCounts();
   std::optional<int> B = llvm::transformOptional(
-      std::move(A), [&](const MoveOnly &M) { return M.val + 2; });
+      std::move(A), [&](const CountCopyAndMove &M) { return M.val + 2; });
   EXPECT_FALSE(B.has_value());
-  EXPECT_EQ(0u, MoveOnly::MoveConstructions);
-  EXPECT_EQ(0u, MoveOnly::MoveAssignments);
-  EXPECT_EQ(0u, MoveOnly::Destructions);
+  EXPECT_EQ(0, CountCopyAndMove::MoveConstructions);
+  EXPECT_EQ(0, CountCopyAndMove::MoveAssignments);
+  EXPECT_EQ(0, CountCopyAndMove::Destructions);
 
-  A = MoveOnly(5);
-  MoveOnly::ResetCounts();
+  A = CountCopyAndMove(5);
+  CountCopyAndMove::ResetCounts();
   std::optional<int> C = llvm::transformOptional(
-      std::move(A), [&](const MoveOnly &M) { return M.val + 2; });
+      std::move(A), [&](const CountCopyAndMove &M) { return M.val + 2; });
   EXPECT_TRUE(C.has_value());
   EXPECT_EQ(7, *C);
-  EXPECT_EQ(0u, MoveOnly::MoveConstructions);
-  EXPECT_EQ(0u, MoveOnly::MoveAssignments);
-  EXPECT_EQ(0u, MoveOnly::Destructions);
+  EXPECT_EQ(0, CountCopyAndMove::MoveConstructions);
+  EXPECT_EQ(0, CountCopyAndMove::MoveAssignments);
+  EXPECT_EQ(0, CountCopyAndMove::Destructions);
 }
 
 TEST(TransformTest, TransformLlvm) {
@@ -96,27 +96,27 @@ TEST(TransformTest, TransformLlvm) {
 }
 
 TEST(TransformTest, MoveTransformLlvm) {
-  using llvm::MoveOnly;
+  using llvm::CountCopyAndMove;
 
-  std::optional<MoveOnly> A;
+  std::optional<CountCopyAndMove> A;
 
-  MoveOnly::ResetCounts();
+  CountCopyAndMove::ResetCounts();
   std::optional<int> B = llvm::transformOptional(
-      std::move(A), [&](const MoveOnly &M) { return M.val + 2; });
+      std::move(A), [&](const CountCopyAndMove &M) { return M.val + 2; });
   EXPECT_FALSE(B.has_value());
-  EXPECT_EQ(0u, MoveOnly::MoveConstructions);
-  EXPECT_EQ(0u, MoveOnly::MoveAssignments);
-  EXPECT_EQ(0u, MoveOnly::Destructions);
+  EXPECT_EQ(0, CountCopyAndMove::MoveConstructions);
+  EXPECT_EQ(0, CountCopyAndMove::MoveAssignments);
+  EXPECT_EQ(0, CountCopyAndMove::Destructions);
 
-  A = MoveOnly(5);
-  MoveOnly::ResetCounts();
+  A = CountCopyAndMove(5);
+  CountCopyAndMove::ResetCounts();
   std::optional<int> C = llvm::transformOptional(
-      std::move(A), [&](const MoveOnly &M) { return M.val + 2; });
+      std::move(A), [&](const CountCopyAndMove &M) { return M.val + 2; });
   EXPECT_TRUE(C.has_value());
   EXPECT_EQ(7, *C);
-  EXPECT_EQ(0u, MoveOnly::MoveConstructions);
-  EXPECT_EQ(0u, MoveOnly::MoveAssignments);
-  EXPECT_EQ(0u, MoveOnly::Destructions);
+  EXPECT_EQ(0, CountCopyAndMove::MoveConstructions);
+  EXPECT_EQ(0, CountCopyAndMove::MoveAssignments);
+  EXPECT_EQ(0, CountCopyAndMove::Destructions);
 }
 
 TEST(TransformTest, ToUnderlying) {

dwblaikie

Looks good, but for the code that was using MoveOnly could you add checks that no copy operations happened there? (Previously the code would've checked that statically, since the type was move only so it would fail to compile)

And so you need me to push this for you?

c8ef · 2024-06-05T02:57:48Z

Looks good, but for the code that was using MoveOnly could you add checks that no copy operations happened there? (Previously the code would've checked that statically, since the type was move only so it would fail to compile)

Assertions added to ensure zero copying.

And so you need me to push this for you?

That would be great, Thanks!

refactor MoveOnly type in ADT ut

2679f8d

llvmbot added the llvm:adt label Jun 5, 2024

c8ef mentioned this pull request Jun 5, 2024

[ADT] Add C++17-style insert_or_assign for DenseMap #94151

Merged

dwblaikie approved these changes Jun 5, 2024

View reviewed changes

make sure no copy in STLForwardCompatTest

3152c94

dwblaikie approved these changes Jun 5, 2024

View reviewed changes

dwblaikie merged commit 6871657 into llvm:main Jun 5, 2024
7 checks passed

c8ef deleted the refactor-adt-test branch June 5, 2024 05:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[ADT] refactor MoveOnly type in ADT unittest #94421

[ADT] refactor MoveOnly type in ADT unittest #94421

Uh oh!

c8ef commented Jun 5, 2024

Uh oh!

llvmbot commented Jun 5, 2024

Uh oh!

dwblaikie left a comment

Uh oh!

c8ef commented Jun 5, 2024

Uh oh!

Uh oh!

Uh oh!

[ADT] refactor MoveOnly type in ADT unittest #94421

[ADT] refactor MoveOnly type in ADT unittest #94421

Uh oh!

Conversation

c8ef commented Jun 5, 2024

Uh oh!

llvmbot commented Jun 5, 2024

Uh oh!

dwblaikie left a comment

Choose a reason for hiding this comment

Uh oh!

c8ef commented Jun 5, 2024

Uh oh!

Uh oh!

Uh oh!