From 73084012e20b6fbc22b8fe20fdb02dcddedf6a9d Mon Sep 17 00:00:00 2001 From: Dimitar Krastev Date: Wed, 26 Jun 2024 07:47:02 +0300 Subject: [PATCH] `PointerVector` smart pointer intergration. (#1468) * Added unique_ptr overloads for PointerVector. * Fixed 'getAndRemove' documentation. * Added sanity check if a nullptr is pushed to the vector. * Typo. * Refactored 'typedef' to 'using' style. * Added constant overloads of front() and back() * Added copy constructor and copy/move assignment operators. * Typo fixes * Removed dead code comment. * Fixed documentation. * Changed ref to const ref. * Suppresses a falce positive danglingLifetime error. * Added missed header. * Lint * Explicitly clearing vector on move to mirror move assignment. * Typo * Changed copy to move assignment. --- Common++/header/PointerVector.h | 210 ++++++++++++++++++++++++++------ 1 file changed, 174 insertions(+), 36 deletions(-) diff --git a/Common++/header/PointerVector.h b/Common++/header/PointerVector.h index ef0c26f012..90f64e0eae 100644 --- a/Common++/header/PointerVector.h +++ b/Common++/header/PointerVector.h @@ -2,7 +2,9 @@ #include #include +#include #include +#include /// @file @@ -26,12 +28,12 @@ namespace pcpp /** * Iterator object that is used for iterating all elements in the vector */ - typedef typename std::vector::iterator VectorIterator; + using VectorIterator = typename std::vector::iterator; /** * Const iterator object that is used for iterating all elements in a constant vector */ - typedef typename std::vector::const_iterator ConstVectorIterator; + using ConstVectorIterator = typename std::vector::const_iterator; /** * A constructor that create an empty instance of this object @@ -39,48 +41,73 @@ namespace pcpp PointerVector() {} + /** + * Copies the vector along with all elements inside it. + * All elements inside the copied vector are duplicates and the originals remain unchanged. + * @param[in] other The vector to copy from. + * @remarks As the vector is copied via deep copy, all pointers obtained from the copied vector + * reference the duplicates and not the originals. + */ + PointerVector(const PointerVector& other) : m_Vector(deepCopyUnsafe(other.m_Vector)) + {} + + /** + * Move constructor. All elements along with their ownership is transferred to the new vector. + * @param[in] other The vector to move from. + */ + PointerVector(PointerVector&& other) noexcept : m_Vector(std::move(other.m_Vector)) + { + other.m_Vector.clear(); + } + /** * A destructor for this class. The destructor frees all elements that are binded to the vector */ ~PointerVector() { - for (auto iter : m_Vector) - { - delete iter; - } + freeVectorUnsafe(m_Vector); } /** - * Copy constructor. Once a vector is copied from another vector, all elements inside it are copied, - * meaning the new vector will contain pointers to copied elements, not pointers to the elements of the original - * vector + * A copy assignment operator. Replaces the contents with a copy of the contents of other. + * See copy constructor for more information on the specific copy procedure. + * @param[in] other The vector to copy from. + * @return A reference to the current object. */ - PointerVector(const PointerVector& other) + PointerVector& operator=(const PointerVector& other) { + // Saves a copy of the old pointer to defer cleanup. + auto oldValues = m_Vector; try { - for (const auto iter : other) - { - T* objCopy = new T(*iter); - try - { - m_Vector.push_back(objCopy); - } - catch (const std::exception&) - { - delete objCopy; - throw; - } - } + m_Vector = deepCopyUnsafe(other.m_Vector); } + // If an exception is thrown during the copy operation, restore old values and rethrow. catch (const std::exception&) { - for (auto obj : m_Vector) - { - delete obj; - } + m_Vector = std::move(oldValues); throw; } + // Free old values as the new ones have been successfully assigned. + freeVectorUnsafe(oldValues); + return *this; + } + + /** + * A move assignment operator. Replaces the contents with those of other via move semantics. + * The other vector is left empty. + * @param[in] other The vector to move from. + * @return A reference to the current object. + */ + PointerVector& operator=(PointerVector&& other) noexcept + { + // Releases all current elements. + clear(); + // Moves the elements of the other vector. + m_Vector = std::move(other.m_Vector); + // Explicitly clear the other vector as the standard only guarantees an unspecified valid state after move. + other.m_Vector.clear(); + return *this; } /** @@ -88,22 +115,45 @@ namespace pcpp */ void clear() { - for (auto iter : m_Vector) - { - delete iter; - } - + freeVectorUnsafe(m_Vector); m_Vector.clear(); } /** * Add a new (pointer to an) element to the vector + * @param[in] element A pointer to an element to assume ownership of. + * @throws std::invalid_argument The provided pointer is a nullptr. */ void pushBack(T* element) { + if (element == nullptr) + { + throw std::invalid_argument("Element is nullptr"); + } + m_Vector.push_back(element); } + /** + * Add a new element to the vector that has been managed by an unique pointer. + * @param[in] element A unique pointer holding an element. + * @throws std::invalid_argument The provided pointer is a nullptr. + * @remarks If pushBack throws the element is freed immediately. + */ + void pushBack(std::unique_ptr element) + { + if (!element) + { + throw std::invalid_argument("Element is nullptr"); + } + + // Release is called after the raw pointer is already inserted into the vector to prevent + // a memory leak if push_back throws. + // cppcheck-suppress danglingLifetime + m_Vector.push_back(element.get()); + element.release(); + } + /** * Get the first element of the vector * @return An iterator object pointing to the first element of the vector @@ -140,8 +190,6 @@ namespace pcpp return m_Vector.end(); } - // inline size_t size() { return m_Vector.size(); } - /** * Get number of elements in the vector * @return The number of elements in the vector @@ -152,7 +200,6 @@ namespace pcpp } /** - * Returns a pointer of the first element in the vector * @return A pointer of the first element in the vector */ T* front() @@ -160,6 +207,14 @@ namespace pcpp return m_Vector.front(); } + /** + * @return A pointer to the first element in the vector + */ + T const* front() const + { + return m_Vector.front(); + } + /** * @return A pointer to the last element in the vector */ @@ -168,6 +223,14 @@ namespace pcpp return m_Vector.back(); } + /* + * @return A pointer to the last element in the vector. + */ + T const* back() const + { + return m_Vector.back(); + } + /** * Removes from the vector a single element (position). Once the element is erased, it's also freed * @param[in] position The position of the element to erase @@ -182,7 +245,8 @@ namespace pcpp /** * Remove an element from the vector without freeing it - * param[in] position The position of the element to remove from the vector + * @param[in, out] position The position of the element to remove from the vector. + * The iterator is shifted to the following element after the removal is completed. * @return A pointer to the element which is no longer managed by the vector. It's user responsibility to free * it */ @@ -195,6 +259,29 @@ namespace pcpp return result; } + /** + * Removes an element from the vector and transfers ownership to the returned unique pointer. + * @param[in] index The index of the element to detach. + * @return An unique pointer that holds ownership of the detached element. + */ + std::unique_ptr getAndDetach(size_t index) + { + return getAndDetach(m_Vector.begin() + index); + } + + /** + * Removes an element from the vector and transfers ownership to the returned unique pointer. + * @param[in, out] position An iterator pointing to the element to detach. + * The iterator is shifted to the following element after the detach completes. + * @return An unique pointer that holds ownership of the detached element. + */ + std::unique_ptr getAndDetach(VectorIterator& position) + { + std::unique_ptr result(*position); + position = m_Vector.erase(position); + return result; + } + /** * Return a pointer to the element in a certain index * @param[in] index The index to retrieve the element from @@ -216,6 +303,57 @@ namespace pcpp } private: + /** + * Performs a copy of the vector along with its elements. + * The caller is responsible of freeing the copied elements. + * @return A vector of pointers to the newly copied elements. + */ + static std::vector deepCopyUnsafe(std::vector const& origin) + { + std::vector copyVec; + + try + { + for (const auto iter : origin) + { + T* objCopy = new T(*iter); + try + { + copyVec.push_back(objCopy); + } + catch (const std::exception&) + { + delete objCopy; + throw; + } + } + } + catch (const std::exception&) + { + for (auto obj : copyVec) + { + delete obj; + } + throw; + } + + return copyVec; + } + + /** + * Frees all elements inside the vector. + * Calling this function with non-heap allocated pointers is UB. + * @param[in] origin The vector of elements to free. + * @remarks The vector's contents are not cleared and will point to invalid locations in memory. + */ + static void freeVectorUnsafe(std::vector const& origin) + { + for (auto& obj : origin) + { + delete obj; + } + } + std::vector m_Vector; };