From 963b232fd0ba6e97220f303a9c51cc4bfe3f37dd Mon Sep 17 00:00:00 2001 From: Xingyang Yuan Date: Thu, 16 Jan 2025 19:59:35 +0800 Subject: [PATCH] iox-#2414 Reset index after move construction --- .../release-notes/iceoryx-unreleased.md | 1 + .../moduletests/test_vocabulary_expected.cpp | 14 ++--- .../moduletests/test_vocabulary_variant.cpp | 55 +++++++++++++++++-- .../vocabulary/include/iox/detail/variant.inl | 3 + 4 files changed, 59 insertions(+), 14 deletions(-) diff --git a/doc/website/release-notes/iceoryx-unreleased.md b/doc/website/release-notes/iceoryx-unreleased.md index 13a3b11c48..d4d2a51d5e 100644 --- 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 index c08097950c..f35078dc5b 100644 --- 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 index 7746ab4ba9..dbeca2673d 100644 --- 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(), 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 schlomo; + schlomo = ComplexClass(2, 3.14F); + iox::variant 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()->a, Eq(2)); + EXPECT_THAT(ignatz.get()->b, Eq(3.14F)); +} + +TEST_F(variant_Test, MoveAssignmentWithDifferentTypeVariantLeadsToSameValue) +{ + ::testing::Test::RecordProperty("TEST_ID", "562a38c3-aac2-4b1f-be55-c2d1b49e6c53"); + iox::variant schlomo; + schlomo = ComplexClass(2, 3.14F); + iox::variant 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()->a, Eq(2)); + EXPECT_THAT(ignatz.get()->b, Eq(3.14F)); +} + +TEST_F(variant_Test, MoveAssignmentWithSameTypeVariantLeadsToSameValue) +{ + ::testing::Test::RecordProperty("TEST_ID", "e4a530af-05c0-49e5-ae04-f3512f299fbe"); + iox::variant schlomo; + schlomo = ComplexClass(2, 3.14F); + iox::variant 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()->a, Eq(2)); + EXPECT_THAT(ignatz.get()->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 index 537dd48a0a..3a15cf4ce1 100644 --- 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::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,6 +115,7 @@ inline constexpr variant& variant::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 @@ -121,6 +123,7 @@ inline constexpr variant& variant::operator=(variant&& rhs) 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; } } }