Skip to content

Commit

Permalink
Merge pull request eclipse-iceoryx#1651 from ApexAI/iox-1643-revert-d…
Browse files Browse the repository at this point in the history 
…estruction-order-in-copy-move-assignment-of-vector

iox-1643 Revert destruction order in copy and move assignment operators of cxx::vector
  • Loading branch information
@FerdinandSpitzschnueffler
FerdinandSpitzschnueffler authored Sep 23, 2022
2 parents f7e8136 + c4358ab commit eef896a
Show file tree
Hide file tree
Showing 3 changed files with 132 additions and 45 deletions.
65 changes: 42 additions & 23 deletions iceoryx_hoofs/include/iceoryx_hoofs/cxx/vector.hpp
View file
Original file line number Diff line number Diff line change
@@ -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.
Expand All @@ -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 <algorithm>
Expand All @@ -40,8 +41,6 @@ template <typename T, uint64_t Capacity>
class vector
{
public:
using value_type = T;

using iterator = T*;
using const_iterator = const T*;

Expand All @@ -58,23 +57,29 @@ 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
/// contained elements
/// contained elements in reverse construction order
~vector() noexcept;

/// @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,
Expand Down Expand Up @@ -103,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
Expand All @@ -157,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.
Expand All @@ -172,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 <typename... Targs>
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 <typename... Targs>
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;

Expand All @@ -195,12 +210,16 @@ 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:
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)];
Expand Down
35 changes: 17 additions & 18 deletions iceoryx_hoofs/include/iceoryx_hoofs/internal/cxx/vector.inl
View file
Original file line number Diff line number Diff line change
@@ -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.
Expand Down Expand Up @@ -103,10 +103,7 @@ inline vector<T, Capacity>& vector<T, Capacity>::operator=(const vector& rhs) no
}

// delete remaining elements
for (; i < size(); ++i)
{
at(i).~T();
}
clearFrom(i);

m_size = rhs.m_size;
}
Expand All @@ -132,10 +129,7 @@ inline vector<T, Capacity>& vector<T, Capacity>::operator=(vector&& rhs) noexcep
}

// delete remaining elements
for (; i < size(); ++i)
{
at(i).~T();
}
clearFrom(i);

m_size = rhs.m_size;
rhs.clear();
Expand Down Expand Up @@ -164,9 +158,7 @@ inline uint64_t vector<T, Capacity>::capacity() const noexcept
template <typename T, uint64_t Capacity>
inline void vector<T, Capacity>::clear() noexcept
{
while (pop_back())
{
}
clearFrom(0);
}

template <typename T, uint64_t Capacity>
Expand Down Expand Up @@ -239,10 +231,7 @@ inline bool vector<T, Capacity>::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)
{
Expand Down Expand Up @@ -370,27 +359,37 @@ inline typename vector<T, Capacity>::iterator vector<T, Capacity>::erase(iterato
}
at(n).~T();
m_size--;
return &at_unchecked(index);
}
return nullptr;
}

template <typename T, uint64_t Capacity>
T& vector<T, Capacity>::at_unchecked(const uint64_t index) noexcept
inline T& vector<T, Capacity>::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)
return const_cast<T&>(const_cast<const vector<T, Capacity>*>(this)->at_unchecked(index));
}

template <typename T, uint64_t Capacity>
const T& vector<T, Capacity>::at_unchecked(const uint64_t index) const noexcept
inline const T& vector<T, Capacity>::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
// NOLINTNEXTLINE(cppcoreguidelines-pro-type-reinterpret-cast,cppcoreguidelines-pro-bounds-pointer-arithmetic)
return reinterpret_cast<const T*>(m_data)[index];
}

template <typename T, uint64_t Capacity>
inline void vector<T, Capacity>::clearFrom(const uint64_t startPosition) noexcept
{
while (m_size > startPosition)
{
at_unchecked(--m_size).~T();
}
}

} // namespace cxx
} // namespace iox

Expand Down
77 changes: 73 additions & 4 deletions iceoryx_hoofs/test/moduletests/test_cxx_vector.cpp
View file
Original file line number Diff line number Diff line change
@@ -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.
Expand Down Expand Up @@ -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));
Expand All @@ -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};
Expand All @@ -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));
Expand Down Expand Up @@ -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<CTorTest, VECTOR_CAPACITY> sut1;
vector<CTorTest, VECTOR_CAPACITY> 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<CTorTest, VECTOR_CAPACITY> sut1;
vector<CTorTest, VECTOR_CAPACITY> 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");
Expand Down Expand Up @@ -874,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()));

iter = sut.erase(sut.begin() + 1);
ASSERT_THAT(sut.size(), Eq(1));
ASSERT_NE(iter, nullptr);
EXPECT_THAT(iter, Eq(sut.end()));

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");
Expand Down

0 comments on commit eef896a

Please sign in to comment.