From eb5f21af10eb14d0cbb535d612c883ed300191df Mon Sep 17 00:00:00 2001 From: Marika Lehmann Date: Mon, 19 Sep 2022 19:34:57 +0200 Subject: [PATCH 1/5] iox-#1643 Revert destruction order in copy and move assignment operators Signed-off-by: Marika Lehmann --- .../include/iceoryx_hoofs/cxx/vector.hpp | 3 +- .../iceoryx_hoofs/internal/cxx/vector.inl | 10 ++--- .../test/moduletests/test_cxx_vector.cpp | 42 ++++++++++++++++++- 3 files changed, 48 insertions(+), 7 deletions(-) diff --git a/iceoryx_hoofs/include/iceoryx_hoofs/cxx/vector.hpp b/iceoryx_hoofs/include/iceoryx_hoofs/cxx/vector.hpp index 8439773284..cdeef65e63 100644 --- a/iceoryx_hoofs/include/iceoryx_hoofs/cxx/vector.hpp +++ b/iceoryx_hoofs/include/iceoryx_hoofs/cxx/vector.hpp @@ -17,6 +17,7 @@ #ifndef IOX_HOOFS_CXX_VECTOR_HPP #define IOX_HOOFS_CXX_VECTOR_HPP +#include "iceoryx_hoofs/cxx/attributes.hpp" #include "iceoryx_hoofs/cxx/requires.hpp" #include @@ -64,7 +65,7 @@ class vector vector(vector&& rhs) noexcept; /// @brief destructs the vector and also calls the destructor of all - /// contained elements + /// contained elements in reverse construction order ~vector() noexcept; /// @brief copy assignment. if the destination vector contains more diff --git a/iceoryx_hoofs/include/iceoryx_hoofs/internal/cxx/vector.inl b/iceoryx_hoofs/include/iceoryx_hoofs/internal/cxx/vector.inl index eccbb9e6eb..2e39e7c2f2 100644 --- a/iceoryx_hoofs/include/iceoryx_hoofs/internal/cxx/vector.inl +++ b/iceoryx_hoofs/include/iceoryx_hoofs/internal/cxx/vector.inl @@ -1,5 +1,5 @@ // Copyright (c) 2019 by Robert Bosch GmbH. All rights reserved. -// Copyright (c) 2021 by Apex.AI Inc. All rights reserved. +// Copyright (c) 2021 - 2022 by Apex.AI Inc. All rights reserved. // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -103,9 +103,9 @@ inline vector& vector::operator=(const vector& rhs) no } // delete remaining elements - for (; i < size(); ++i) + while (i < size()) { - at(i).~T(); + IOX_DISCARD_RESULT(pop_back()); } m_size = rhs.m_size; @@ -132,9 +132,9 @@ inline vector& vector::operator=(vector&& rhs) noexcep } // delete remaining elements - for (; i < size(); ++i) + while (i < size()) { - at(i).~T(); + IOX_DISCARD_RESULT(pop_back()); } m_size = rhs.m_size; diff --git a/iceoryx_hoofs/test/moduletests/test_cxx_vector.cpp b/iceoryx_hoofs/test/moduletests/test_cxx_vector.cpp index de0d2a9147..30f4c3edc7 100644 --- a/iceoryx_hoofs/test/moduletests/test_cxx_vector.cpp +++ b/iceoryx_hoofs/test/moduletests/test_cxx_vector.cpp @@ -1,5 +1,5 @@ // Copyright (c) 2019 by Robert Bosch GmbH. All rights reserved. -// Copyright (c) 2021 by Apex.AI Inc. All rights reserved. +// Copyright (c) 2021 - 2022 by Apex.AI Inc. All rights reserved. // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -565,6 +565,46 @@ TEST_F(vector_test, CopyAssignmentWithLargerSource) EXPECT_THAT(sut2.at(3U).value, Eq(158432U)); } +TEST_F(vector_test, ReverseDestructionOrderInCopyAssignment) +{ + ::testing::Test::RecordProperty("TEST_ID", "00ba138d-a805-4261-ac54-5eeea605e50c"); + constexpr uint64_t VECTOR_CAPACITY{10}; + vector sut1; + vector sut2; + for (uint64_t i{0}; i < VECTOR_CAPACITY; ++i) + { + sut1.emplace_back(i); + } + sut1 = sut2; + + EXPECT_THAT(dTor, Eq(VECTOR_CAPACITY)); + ASSERT_THAT(dtorOrder.size(), Eq(VECTOR_CAPACITY)); + for (uint64_t i{0}; i < VECTOR_CAPACITY; ++i) + { + EXPECT_THAT(dtorOrder[i], Eq(VECTOR_CAPACITY - 1 - i)); + } +} + +TEST_F(vector_test, ReverseDestructionOrderInMoveAssignment) +{ + ::testing::Test::RecordProperty("TEST_ID", "7a523770-7eab-4405-a9c1-a1b451534eb0"); + constexpr uint64_t VECTOR_CAPACITY{10}; + vector sut1; + vector sut2; + for (uint64_t i{0}; i < VECTOR_CAPACITY; ++i) + { + sut1.emplace_back(i + 1); + } + sut1 = std::move(sut2); + + EXPECT_THAT(dTor, Eq(VECTOR_CAPACITY)); + ASSERT_THAT(dtorOrder.size(), Eq(VECTOR_CAPACITY)); + for (uint64_t i{0}; i < VECTOR_CAPACITY; ++i) + { + EXPECT_THAT(dtorOrder[i], Eq(VECTOR_CAPACITY - i)); + } +} + TEST_F(vector_test, MoveAssignmentWithEmptySource) { ::testing::Test::RecordProperty("TEST_ID", "dc8c2211-e8f6-4a49-a1bb-8344894c017b"); From 9e70e7b15d8219922e1eccfc2768ba08030227e1 Mon Sep 17 00:00:00 2001 From: Marika Lehmann Date: Wed, 21 Sep 2022 14:24:02 +0200 Subject: [PATCH 2/5] iox-#1643 Fix erase method, complete doxygen documentation Signed-off-by: Marika Lehmann --- .../include/iceoryx_hoofs/cxx/vector.hpp | 58 ++++++++++++------- .../iceoryx_hoofs/internal/cxx/vector.inl | 9 +++ .../test/moduletests/test_cxx_vector.cpp | 35 ++++++++++- 3 files changed, 78 insertions(+), 24 deletions(-) diff --git a/iceoryx_hoofs/include/iceoryx_hoofs/cxx/vector.hpp b/iceoryx_hoofs/include/iceoryx_hoofs/cxx/vector.hpp index cdeef65e63..5ff2411ade 100644 --- a/iceoryx_hoofs/include/iceoryx_hoofs/cxx/vector.hpp +++ b/iceoryx_hoofs/include/iceoryx_hoofs/cxx/vector.hpp @@ -41,8 +41,6 @@ template class vector { public: - using value_type = T; - using iterator = T*; using const_iterator = const T*; @@ -59,9 +57,11 @@ class vector explicit vector(const uint64_t count) noexcept; /// @brief copy constructor to copy a vector of the same capacity + /// @param[in] rhs is the copy origin vector(const vector& rhs) noexcept; /// @brief move constructor to move a vector of the same capacity + /// @param[in] rhs is the move origin vector(vector&& rhs) noexcept; /// @brief destructs the vector and also calls the destructor of all @@ -71,11 +71,15 @@ class vector /// @brief copy assignment. if the destination vector contains more /// elements than the source the remaining elements will be /// destructed + /// @param[in] rhs is the copy origin + /// @return reference to self vector& operator=(const vector& rhs) noexcept; /// @brief move assignment. if the destination vector contains more /// elements than the source the remaining elements will be /// destructed + /// @param[in] rhs is the move origin + /// @return reference to self vector& operator=(vector&& rhs) noexcept; /// @brief returns an iterator to the first element of the vector, @@ -104,44 +108,48 @@ class vector /// @return const pointer to underlying array const T* data() const noexcept; - /// @brief returns a reference to the element stored at index. the behavior - // is undefined if the element at index does not exist. - /// @attention Out of bounds access can lead to a program termination! + /// @brief returns a reference to the element stored at index. + /// @param[in] index of the element to return + /// @return reference to the element stored at index + /// @attention Out of bounds access leads to a program termination! T& at(const uint64_t index) noexcept; - /// @brief returns a const reference to the element stored at index. the - /// behavior is undefined if the element at index does not exist. - /// @attention Out of bounds access can lead to a program termination! + /// @brief returns a const reference to the element stored at index. + /// @param[in] index of the element to return + /// @return const reference to the element stored at index + /// @attention Out of bounds access leads to a program termination! const T& at(const uint64_t index) const noexcept; - /// @brief returns a reference to the element stored at index. the behavior - // is undefined if the element at index does not exist. - /// @attention Out of bounds access can lead to a program termination! + /// @brief returns a reference to the element stored at index. + /// @param[in] index of the element to return + /// @return reference to the element stored at index + /// @attention Out of bounds access leads to a program termination! T& operator[](const uint64_t index) noexcept; - /// @brief returns a const reference to the element stored at index. the - /// behavior is undefined if the element at index does not exist. - /// @attention Out of bounds access can lead to a program termination! + /// @brief returns a const reference to the element stored at index. + /// @param[in] index of the element to return + /// @return const reference to the element stored at index + /// @attention Out of bounds access leads to a program termination! const T& operator[](const uint64_t index) const noexcept; /// @brief returns a reference to the first element; terminates if the vector is empty /// @return reference to the first element - /// @attention Accessing an empty vector can lead to a program termination! + /// @attention Accessing an empty vector leads to a program termination! T& front() noexcept; /// @brief returns a const reference to the first element; terminates if the vector is empty /// @return const reference to the first element - /// @attention Accessing an empty vector can lead to a program termination! + /// @attention Accessing an empty vector leads to a program termination! const T& front() const noexcept; /// @brief returns a reference to the last element; terminates if the vector is empty /// @return reference to the last element - /// @attention Accessing an empty vector can lead to a program termination! + /// @attention Accessing an empty vector leads to a program termination! T& back() noexcept; /// @brief returns a const reference to the last element; terminates if the vector is empty /// @return const reference to the last element - /// @attention Accessing an empty vector can lead to a program termination! + /// @attention Accessing an empty vector leads to a program termination! const T& back() const noexcept; /// @brief returns the capacity of the vector which was given via the template @@ -158,9 +166,9 @@ class vector /// @brief calls the destructor of all contained elements and removes them void clear() noexcept; - /// @brief resizes the vector. If the vector size increases new elements - /// will be constructed with the given arguments. If count is greater than the capacity - /// the vector will stay unchanged. + /// @brief resizes the vector. If the vector size increases new elements will be constructed with the given + /// arguments. If count is greater than the capacity the vector will stay unchanged. If count is less than the size, + /// the remaining elements will be removed and no new elements will be constructed. /// @param[in] count new size of the vector /// @param[in] args arguments which are used by the constructor of newly created elements /// @return true if the resize was successful, false if count is greater than the capacity. @@ -173,19 +181,25 @@ class vector /// @brief forwards all arguments to the constructor of the contained element /// and performs a placement new at the provided position /// @param[in] position the position where the element should be created + /// @param[in] args arguments which are used by the constructor of the newly created argument + /// @return true if successful, false if position is greater than size or the vector is already full template bool emplace(const uint64_t position, Targs&&... args) noexcept; /// @brief forwards all arguments to the constructor of the contained element /// and performs a placement new at the end + /// @param[in] args arguments which are used by the constructor of the newly created argument + /// @return true if successful, false if the vector is already full template bool emplace_back(Targs&&... args) noexcept; /// @brief appends the given element at the end of the vector + /// @param[in] value to append to the vector /// @return true if successful, false if vector already full bool push_back(const T& value) noexcept; /// @brief appends the given element at the end of the vector + /// @param[in] value to append to the vector /// @return true if successful, false if vector already full bool push_back(T&& value) noexcept; @@ -196,6 +210,8 @@ class vector /// @brief removes an element at the given position. if this element is in /// the middle of the vector every element is moved one place to the /// left to ensure that the elements are stored contiguously + /// @param[in] position at which the element shall be removed + /// @return iterator following the removed element if begin() <= position < end(), otherwise nullptr iterator erase(iterator position) noexcept; private: diff --git a/iceoryx_hoofs/include/iceoryx_hoofs/internal/cxx/vector.inl b/iceoryx_hoofs/include/iceoryx_hoofs/internal/cxx/vector.inl index 2e39e7c2f2..abc499a498 100644 --- a/iceoryx_hoofs/include/iceoryx_hoofs/internal/cxx/vector.inl +++ b/iceoryx_hoofs/include/iceoryx_hoofs/internal/cxx/vector.inl @@ -370,6 +370,15 @@ inline typename vector::iterator vector::erase(iterato } at(n).~T(); m_size--; + if (m_size == 0) + { + return end(); + } + if (index == 0) + { + return begin(); + } + return &at(index - 1); } return nullptr; } diff --git a/iceoryx_hoofs/test/moduletests/test_cxx_vector.cpp b/iceoryx_hoofs/test/moduletests/test_cxx_vector.cpp index 30f4c3edc7..88c4349073 100644 --- a/iceoryx_hoofs/test/moduletests/test_cxx_vector.cpp +++ b/iceoryx_hoofs/test/moduletests/test_cxx_vector.cpp @@ -197,7 +197,7 @@ TEST_F(vector_test, NewVectorWithElementsCTorWithMoreThanCapacityElements) } } -TEST_F(vector_test, EmplaceBackSuccessfullWhenSpaceAvailable) +TEST_F(vector_test, EmplaceBackSuccessfulWhenSpaceAvailable) { ::testing::Test::RecordProperty("TEST_ID", "98d17e04-0d2b-4575-a1f0-7b3cd918c54d"); EXPECT_THAT(sut.emplace_back(5U), Eq(true)); @@ -213,7 +213,7 @@ TEST_F(vector_test, EmplaceBackFailsWhenSpaceNotAvailable) EXPECT_THAT(sut.emplace_back(5U), Eq(false)); } -TEST_F(vector_test, PushBackSuccessfullWhenSpaceAvailableLValue) +TEST_F(vector_test, PushBackSuccessfulWhenSpaceAvailableLValue) { ::testing::Test::RecordProperty("TEST_ID", "42102325-91fa-45aa-a5cb-2bce785d11c1"); const int a{5}; @@ -233,7 +233,7 @@ TEST_F(vector_test, PushBackFailsWhenSpaceNotAvailableLValue) EXPECT_THAT(sut.push_back(a), Eq(false)); } -TEST_F(vector_test, PushBackSuccessfullWhenSpaceAvailableRValue) +TEST_F(vector_test, PushBackSuccessfulWhenSpaceAvailableRValue) { ::testing::Test::RecordProperty("TEST_ID", "47988e05-9c67-4b34-bdee-994552df3fa7"); EXPECT_THAT(sut.push_back(5U), Eq(true)); @@ -914,6 +914,35 @@ TEST_F(vector_test, EraseReturnsNullWhenElementIsInvalid) EXPECT_THAT(sut.erase(i), Eq(nullptr)); } +TEST_F(vector_test, EraseReturnsCorrectIteratorWhenElementIsValid) +{ + ::testing::Test::RecordProperty("TEST_ID", "4ebc10a8-8cb3-4151-aa70-824d4c0b5597"); + sut.emplace_back(1U); + sut.emplace_back(2U); + sut.emplace_back(3U); + sut.emplace_back(4U); + + auto* iter = sut.erase(sut.begin()); + ASSERT_THAT(sut.size(), Eq(3)); + ASSERT_NE(iter, nullptr); + EXPECT_THAT(iter, Eq(sut.begin())); + + iter = sut.erase(sut.end() - 1); + ASSERT_THAT(sut.size(), Eq(2)); + ASSERT_NE(iter, nullptr); + EXPECT_THAT(iter, Eq(sut.end() - 1)); + + iter = sut.erase(sut.begin() + 1); + ASSERT_THAT(sut.size(), Eq(1)); + ASSERT_NE(iter, nullptr); + EXPECT_THAT(iter, Eq(sut.begin())); + + iter = sut.erase(sut.begin()); + ASSERT_THAT(sut.size(), Eq(0)); + ASSERT_NE(iter, nullptr); + EXPECT_THAT(iter, Eq(sut.end())); +} + TEST_F(vector_test, ErasingElementDecreasesSize) { ::testing::Test::RecordProperty("TEST_ID", "713074f9-0ad1-446e-a2a1-0707dcc112ca"); From d22f5ce0bf35c5bb9f1dc4411621c5e3eea28fea Mon Sep 17 00:00:00 2001 From: Marika Lehmann Date: Wed, 21 Sep 2022 15:12:21 +0200 Subject: [PATCH 3/5] iox-#1643 Add private clearFrom method Signed-off-by: Marika Lehmann --- .../include/iceoryx_hoofs/cxx/vector.hpp | 4 ++- .../iceoryx_hoofs/internal/cxx/vector.inl | 28 +++++++++---------- 2 files changed, 16 insertions(+), 16 deletions(-) diff --git a/iceoryx_hoofs/include/iceoryx_hoofs/cxx/vector.hpp b/iceoryx_hoofs/include/iceoryx_hoofs/cxx/vector.hpp index 5ff2411ade..c256abae3f 100644 --- a/iceoryx_hoofs/include/iceoryx_hoofs/cxx/vector.hpp +++ b/iceoryx_hoofs/include/iceoryx_hoofs/cxx/vector.hpp @@ -1,5 +1,5 @@ // Copyright (c) 2019 by Robert Bosch GmbH. All rights reserved. -// Copyright (c) 2021 by Apex.AI Inc. All rights reserved. +// Copyright (c) 2021 - 2022 by Apex.AI Inc. All rights reserved. // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -218,6 +218,8 @@ class vector T& at_unchecked(const uint64_t index) noexcept; const T& at_unchecked(const uint64_t index) const noexcept; + void clearFrom(const uint64_t startPosition) noexcept; + /// @todo #1196 Replace with UninitializedArray // NOLINTBEGIN(cppcoreguidelines-avoid-c-arrays,hicpp-avoid-c-arrays) using element_t = uint8_t[sizeof(T)]; diff --git a/iceoryx_hoofs/include/iceoryx_hoofs/internal/cxx/vector.inl b/iceoryx_hoofs/include/iceoryx_hoofs/internal/cxx/vector.inl index abc499a498..54b2c31dac 100644 --- a/iceoryx_hoofs/include/iceoryx_hoofs/internal/cxx/vector.inl +++ b/iceoryx_hoofs/include/iceoryx_hoofs/internal/cxx/vector.inl @@ -103,10 +103,7 @@ inline vector& vector::operator=(const vector& rhs) no } // delete remaining elements - while (i < size()) - { - IOX_DISCARD_RESULT(pop_back()); - } + clearFrom(i); m_size = rhs.m_size; } @@ -132,10 +129,7 @@ inline vector& vector::operator=(vector&& rhs) noexcep } // delete remaining elements - while (i < size()) - { - IOX_DISCARD_RESULT(pop_back()); - } + clearFrom(i); m_size = rhs.m_size; rhs.clear(); @@ -164,9 +158,7 @@ inline uint64_t vector::capacity() const noexcept template inline void vector::clear() noexcept { - while (pop_back()) - { - } + clearFrom(0); } template @@ -239,10 +231,7 @@ inline bool vector::resize(const uint64_t count, const Targs&... ar if (count < m_size) { - while (count != m_size) - { - pop_back(); - } + clearFrom(count); } else if (count > m_size) { @@ -400,6 +389,15 @@ const T& vector::at_unchecked(const uint64_t index) const noexcept return reinterpret_cast(m_data)[index]; } +template +void vector::clearFrom(const uint64_t startPosition) noexcept +{ + while (m_size > startPosition) + { + at_unchecked(--m_size).~T(); + } +} + } // namespace cxx } // namespace iox From 0125bc7d6a2e7e073f11c2408377d4dc16d84484 Mon Sep 17 00:00:00 2001 From: Marika Lehmann Date: Wed, 21 Sep 2022 15:49:16 +0200 Subject: [PATCH 4/5] iox-#1643 Add inline keyword Signed-off-by: Marika Lehmann --- iceoryx_hoofs/include/iceoryx_hoofs/internal/cxx/vector.inl | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/iceoryx_hoofs/include/iceoryx_hoofs/internal/cxx/vector.inl b/iceoryx_hoofs/include/iceoryx_hoofs/internal/cxx/vector.inl index 54b2c31dac..13d5ab569a 100644 --- a/iceoryx_hoofs/include/iceoryx_hoofs/internal/cxx/vector.inl +++ b/iceoryx_hoofs/include/iceoryx_hoofs/internal/cxx/vector.inl @@ -373,7 +373,7 @@ inline typename vector::iterator vector::erase(iterato } template -T& vector::at_unchecked(const uint64_t index) noexcept +inline T& vector::at_unchecked(const uint64_t index) noexcept { // AXIVION Next Construct AutosarC++19_03-A5.2.3 : const cast to avoid code duplication // NOLINTNEXTLINE(cppcoreguidelines-pro-type-const-cast) @@ -381,7 +381,7 @@ T& vector::at_unchecked(const uint64_t index) noexcept } template -const T& vector::at_unchecked(const uint64_t index) const noexcept +inline const T& vector::at_unchecked(const uint64_t index) const noexcept { // AXIVION Next Construct AutosarC++19_03-A5.2.4 : Type-safety ensured by template parameter // NOLINTJUSTIFICATION User accessible method at() performs bounds check @@ -390,7 +390,7 @@ const T& vector::at_unchecked(const uint64_t index) const noexcept } template -void vector::clearFrom(const uint64_t startPosition) noexcept +inline void vector::clearFrom(const uint64_t startPosition) noexcept { while (m_size > startPosition) { From c4358ab4b6318ec501448102755cc1295fc35c89 Mon Sep 17 00:00:00 2001 From: Marika Lehmann Date: Fri, 23 Sep 2022 10:38:09 +0200 Subject: [PATCH 5/5] iox-#1643 Fix erase method Signed-off-by: Marika Lehmann --- .../include/iceoryx_hoofs/internal/cxx/vector.inl | 10 +--------- iceoryx_hoofs/test/moduletests/test_cxx_vector.cpp | 4 ++-- 2 files changed, 3 insertions(+), 11 deletions(-) diff --git a/iceoryx_hoofs/include/iceoryx_hoofs/internal/cxx/vector.inl b/iceoryx_hoofs/include/iceoryx_hoofs/internal/cxx/vector.inl index 13d5ab569a..7cca87375e 100644 --- a/iceoryx_hoofs/include/iceoryx_hoofs/internal/cxx/vector.inl +++ b/iceoryx_hoofs/include/iceoryx_hoofs/internal/cxx/vector.inl @@ -359,15 +359,7 @@ inline typename vector::iterator vector::erase(iterato } at(n).~T(); m_size--; - if (m_size == 0) - { - return end(); - } - if (index == 0) - { - return begin(); - } - return &at(index - 1); + return &at_unchecked(index); } return nullptr; } diff --git a/iceoryx_hoofs/test/moduletests/test_cxx_vector.cpp b/iceoryx_hoofs/test/moduletests/test_cxx_vector.cpp index 88c4349073..66c1adb2b4 100644 --- a/iceoryx_hoofs/test/moduletests/test_cxx_vector.cpp +++ b/iceoryx_hoofs/test/moduletests/test_cxx_vector.cpp @@ -930,12 +930,12 @@ TEST_F(vector_test, EraseReturnsCorrectIteratorWhenElementIsValid) iter = sut.erase(sut.end() - 1); ASSERT_THAT(sut.size(), Eq(2)); ASSERT_NE(iter, nullptr); - EXPECT_THAT(iter, Eq(sut.end() - 1)); + EXPECT_THAT(iter, Eq(sut.end())); iter = sut.erase(sut.begin() + 1); ASSERT_THAT(sut.size(), Eq(1)); ASSERT_NE(iter, nullptr); - EXPECT_THAT(iter, Eq(sut.begin())); + EXPECT_THAT(iter, Eq(sut.end())); iter = sut.erase(sut.begin()); ASSERT_THAT(sut.size(), Eq(0));