iox-eclipse-iceoryx#2414 Reset index after move construction

yuanxingyang · Jan 19, 2025 · 963b232 · 963b232
1 parent f33d582
commit 963b232
Show file tree

Hide file tree

Showing 4 changed files with 59 additions and 14 deletions.
diff --git a/doc/website/release-notes/iceoryx-unreleased.md b/doc/website/release-notes/iceoryx-unreleased.md
@@ -148,6 +148,7 @@
 - Depend on @ncurses when building with Bazel [#2372](https://github.com/eclipse-iceoryx/iceoryx/issues/2372)
 - Fix windows performance issue [#2377](https://github.com/eclipse-iceoryx/iceoryx/issues/2377)
 - Max Client and Server cannot be configured independently of Max Publisher and Subscriber [#2394](https://github.com/eclipse-iceoryx/iceoryx/issues/2394)
+- Fix issue where 'variant' index is not reset after move [#2414](https://github.com/eclipse-iceoryx/iceoryx/issues/2414)
 
 **Refactoring:**
 

diff --git a/iceoryx_hoofs/test/moduletests/test_vocabulary_expected.cpp b/iceoryx_hoofs/test/moduletests/test_vocabulary_expected.cpp
@@ -321,8 +321,7 @@ TEST_F(expected_test, CreateWithValueAndMoveCtorLeadsToMovedSource)
 
     // NOLINTJUSTIFICATION we explicitly want to test the defined state of a moved expected
     // NOLINTBEGIN(bugprone-use-after-move,hicpp-invalid-access-moved,clang-analyzer-cplusplus.Move)
-    ASSERT_TRUE(sutSource.has_value());
-    EXPECT_TRUE(sutSource.value().m_moved);
+    ASSERT_FALSE(sutSource.has_value());
     // NOLINTEND(bugprone-use-after-move,hicpp-invalid-access-moved,clang-analyzer-cplusplus.Move)
     ASSERT_TRUE(sutDestination.has_value());
     EXPECT_FALSE(sutDestination.value().m_moved);
@@ -340,8 +339,8 @@ TEST_F(expected_test, CreateWithErrorAndMoveCtorLeadsToMovedSource)
 
     // NOLINTJUSTIFICATION we explicitly want to test the defined state of a moved expected
     // NOLINTBEGIN(bugprone-use-after-move,hicpp-invalid-access-moved,clang-analyzer-cplusplus.Move)
-    ASSERT_TRUE(sutSource.has_error());
-    EXPECT_TRUE(sutSource.error().m_moved);
+    ASSERT_FALSE(sutSource.has_error());
+    ASSERT_FALSE(sutSource.has_value());
     // NOLINTEND(bugprone-use-after-move,hicpp-invalid-access-moved,clang-analyzer-cplusplus.Move)
     ASSERT_TRUE(sutDestination.has_error());
     EXPECT_FALSE(sutDestination.error().m_moved);
@@ -359,8 +358,7 @@ TEST_F(expected_test, CreateWithValueAndMoveAssignmentLeadsToMovedSource)
 
     // NOLINTJUSTIFICATION we explicitly want to test the defined state of a moved expected
     // NOLINTBEGIN(bugprone-use-after-move,hicpp-invalid-access-moved,clang-analyzer-cplusplus.Move)
-    ASSERT_TRUE(sutSource.has_value());
-    EXPECT_TRUE(sutSource.value().m_moved);
+    ASSERT_FALSE(sutSource.has_value());
     // NOLINTEND(bugprone-use-after-move,hicpp-invalid-access-moved,clang-analyzer-cplusplus.Move)
     ASSERT_TRUE(sutDestination.has_value());
     EXPECT_FALSE(sutDestination.value().m_moved);
@@ -378,8 +376,8 @@ TEST_F(expected_test, CreateWithErrorAndMoveAssignmentLeadsToMovedSource)
 
     // NOLINTJUSTIFICATION we explicitly want to test the defined state of a moved expected
     // NOLINTBEGIN(bugprone-use-after-move,hicpp-invalid-access-moved,clang-analyzer-cplusplus.Move)
-    ASSERT_TRUE(sutSource.has_error());
-    EXPECT_TRUE(sutSource.error().m_moved);
+    ASSERT_FALSE(sutSource.has_error());
+    ASSERT_FALSE(sutSource.has_value());
     // NOLINTEND(bugprone-use-after-move,hicpp-invalid-access-moved,clang-analyzer-cplusplus.Move)
     ASSERT_TRUE(sutDestination.has_error());
     EXPECT_FALSE(sutDestination.error().m_moved);

diff --git a/iceoryx_hoofs/test/moduletests/test_vocabulary_variant.cpp b/iceoryx_hoofs/test/moduletests/test_vocabulary_variant.cpp
@@ -341,7 +341,7 @@ TEST_F(variant_Test, MoveCTorWithValueLeadsToSameValue)
     EXPECT_THAT(*ignatz.get<int>(), Eq(123));
     // NOLINTJUSTIFICATION check if move is invalidating the object
     // NOLINTNEXTLINE(bugprone-use-after-move,hicpp-invalid-access-moved,clang-analyzer-cplusplus.Move)
-    EXPECT_THAT(schlomo.index(), Eq(0U));
+    EXPECT_THAT(schlomo.index(), Eq(iox::INVALID_VARIANT_INDEX));
 }
 
 TEST_F(variant_Test, MoveCTorWithoutValueResultsInInvalidVariant)
@@ -352,6 +352,48 @@ TEST_F(variant_Test, MoveCTorWithoutValueResultsInInvalidVariant)
     ASSERT_THAT(ignatz.index(), Eq(iox::INVALID_VARIANT_INDEX));
 }
 
+TEST_F(variant_Test, MoveCTorWithVariantLeadToSameValue)
+{
+    ::testing::Test::RecordProperty("TEST_ID", "dc2a2aff-1fcd-4679-9bfc-b2fb4d2ae928");
+    iox::variant<int, float, ComplexClass> schlomo;
+    schlomo = ComplexClass(2, 3.14F);
+    iox::variant<int, float, ComplexClass> ignatz(std::move(schlomo));
+    // NOLINTJUSTIFICATION check if move is invalidating the object
+    // NOLINTNEXTLINE(bugprone-use-after-move,hicpp-invalid-access-moved,clang-analyzer-cplusplus.Move)
+    ASSERT_THAT(schlomo.index(), Eq(iox::INVALID_VARIANT_INDEX));
+    EXPECT_THAT(ignatz.get<ComplexClass>()->a, Eq(2));
+    EXPECT_THAT(ignatz.get<ComplexClass>()->b, Eq(3.14F));
+}
+
+TEST_F(variant_Test, MoveAssignmentWithDifferentTypeVariantLeadsToSameValue)
+{
+    ::testing::Test::RecordProperty("TEST_ID", "562a38c3-aac2-4b1f-be55-c2d1b49e6c53");
+    iox::variant<int, float, ComplexClass> schlomo;
+    schlomo = ComplexClass(2, 3.14F);
+    iox::variant<int, float, ComplexClass> ignatz(2.14F);
+    ignatz = std::move(schlomo);
+    // NOLINTJUSTIFICATION check if move is invalidating the object
+    // NOLINTNEXTLINE(bugprone-use-after-move,hicpp-invalid-access-moved,clang-analyzer-cplusplus.Move)
+    ASSERT_THAT(schlomo.index(), Eq(iox::INVALID_VARIANT_INDEX));
+    EXPECT_THAT(ignatz.get<ComplexClass>()->a, Eq(2));
+    EXPECT_THAT(ignatz.get<ComplexClass>()->b, Eq(3.14F));
+}
+
+TEST_F(variant_Test, MoveAssignmentWithSameTypeVariantLeadsToSameValue)
+{
+    ::testing::Test::RecordProperty("TEST_ID", "e4a530af-05c0-49e5-ae04-f3512f299fbe");
+    iox::variant<int, float, ComplexClass> schlomo;
+    schlomo = ComplexClass(2, 3.14F);
+    iox::variant<int, float, ComplexClass> ignatz;
+    ignatz = ComplexClass(3, 4.14F);
+    ignatz = std::move(schlomo);
+    // NOLINTJUSTIFICATION check if move is invalidating the object
+    // NOLINTNEXTLINE(bugprone-use-after-move,hicpp-invalid-access-moved,clang-analyzer-cplusplus.Move)
+    ASSERT_THAT(schlomo.index(), Eq(iox::INVALID_VARIANT_INDEX));
+    EXPECT_THAT(ignatz.get<ComplexClass>()->a, Eq(2));
+    EXPECT_THAT(ignatz.get<ComplexClass>()->b, Eq(3.14F));
+}
+
 TEST_F(variant_Test, MoveAssignmentWithValueLeadsToSameValue)
 {
     ::testing::Test::RecordProperty("TEST_ID", "ee36df28-545f-42bc-9ef6-3699284f1a42");
@@ -429,12 +471,12 @@ TEST_F(variant_Test, CreatingSecondObjectViaMoveCTorResultsInTwoDTorCalls)
             EXPECT_THAT(DTorTest::dtorWasCalled, Eq(false));
             // NOLINTJUSTIFICATION check if move is invalidating the object
             // NOLINTNEXTLINE(bugprone-use-after-move,hicpp-invalid-access-moved,clang-analyzer-cplusplus.Move)
-            EXPECT_THAT(ignatz.index(), Eq(1U));
+            EXPECT_THAT(ignatz.index(), Eq(iox::INVALID_VARIANT_INDEX));
         }
         EXPECT_THAT(DTorTest::dtorWasCalled, Eq(true));
         DTorTest::dtorWasCalled = false;
     }
-    EXPECT_THAT(DTorTest::dtorWasCalled, Eq(true));
+    EXPECT_THAT(DTorTest::dtorWasCalled, Eq(false));
 }
 
 TEST_F(variant_Test, CreatingSecondObjectViaMoveAssignmentResultsInTwoDTorCalls)
@@ -450,13 +492,14 @@ TEST_F(variant_Test, CreatingSecondObjectViaMoveAssignmentResultsInTwoDTorCalls)
             schlomo = std::move(ignatz);
             // NOLINTJUSTIFICATION check if move is invalidating the object
             // NOLINTNEXTLINE(bugprone-use-after-move,hicpp-invalid-access-moved,clang-analyzer-cplusplus.Move)
-            EXPECT_THAT(ignatz.index(), Eq(1U));
-            EXPECT_THAT(DTorTest::dtorWasCalled, Eq(false));
+            EXPECT_THAT(ignatz.index(), Eq(iox::INVALID_VARIANT_INDEX));
+            DTorTest::dtorWasCalled = false;
         }
+        // schlomo is destroyed when it goes out of scope
         EXPECT_THAT(DTorTest::dtorWasCalled, Eq(true));
         DTorTest::dtorWasCalled = false;
     }
-    EXPECT_THAT(DTorTest::dtorWasCalled, Eq(true));
+    EXPECT_THAT(DTorTest::dtorWasCalled, Eq(false));
 }
 
 TEST_F(variant_Test, DirectValueAssignmentResultsInCorrectIndex)

diff --git a/iceoryx_hoofs/vocabulary/include/iox/detail/variant.inl b/iceoryx_hoofs/vocabulary/include/iox/detail/variant.inl
@@ -97,6 +97,7 @@ inline constexpr variant<Types...>::variant(variant&& rhs) noexcept
     if (m_type_index != INVALID_VARIANT_INDEX)
     {
         internal::call_at_index<0, Types...>::moveConstructor(m_type_index, &rhs.m_storage, &m_storage);
+        rhs.m_type_index = INVALID_VARIANT_INDEX;
     }
 }
 
@@ -114,13 +115,15 @@ inline constexpr variant<Types...>& variant<Types...>::operator=(variant&& rhs)
             if (m_type_index != INVALID_VARIANT_INDEX)
             {
                 internal::call_at_index<0, Types...>::moveConstructor(m_type_index, &rhs.m_storage, &m_storage);
+                rhs.m_type_index = INVALID_VARIANT_INDEX;
             }
         }
         else
         {
             if (m_type_index != INVALID_VARIANT_INDEX)
             {
                 internal::call_at_index<0, Types...>::move(m_type_index, &rhs.m_storage, &m_storage);
+                rhs.m_type_index = INVALID_VARIANT_INDEX;
             }
         }
     }