Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PointerVector smart pointer intergration. #1468

Merged
merged 20 commits into from
Jun 26, 2024

Conversation

Dimi1010
Copy link
Collaborator

@Dimi1010 Dimi1010 commented Jun 24, 2024

Core changes to PointerVector:

  • Added overload to pushBack that accepts unique_ptr<T>.
  • Added new methods getAndDetach that work analogous to getAndRemoveFromVector but return unique_ptr<T>.
  • Added move constructor that that transfers the ownership of the elements moved to the vector.
  • Added copy/move assignment operators.
  • Added const overloads to front()/back() accessors.
  • Added exceptions when nullptr is being pushed into the vector.

@Dimi1010 Dimi1010 marked this pull request as draft June 24, 2024 16:09
@Dimi1010 Dimi1010 marked this pull request as ready for review June 24, 2024 16:44
Copy link
Collaborator Author

@Dimi1010 Dimi1010 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just adding some reminders of things I noticed, that I should probably fix.

Common++/header/PointerVector.h Outdated Show resolved Hide resolved
Common++/header/PointerVector.h Show resolved Hide resolved
Common++/header/PointerVector.h Outdated Show resolved Hide resolved
@@ -56,7 +56,9 @@ namespace pcpp
* @param[in] other The vector to move from.
*/
PointerVector(PointerVector&& other) noexcept : m_Vector(std::move(other.m_Vector))
{}
{
other.m_Vector.clear();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems redundant, because when people call moving constructor, they suppose to not to use the object anymore?

Copy link
Collaborator Author

@Dimi1010 Dimi1010 Jun 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

std::vector's move ctor technically does not have a well defined state after move. The standard just says it should be in a valid state, but it does not guarantee it will always be empty state. It usually is, but that is implementation defined.

If for some reason, an implementation left the vector non-empty, the destructor will attempt to iterate through the values and call delete on them.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. thanks for the explanation.

@seladb seladb changed the title PoiterVector smart pointer intergration. PointerVector smart pointer intergration. Jun 26, 2024
@tigercosmos tigercosmos merged commit 7308401 into seladb:dev Jun 26, 2024
39 checks passed
@Dimi1010 Dimi1010 deleted the feature/pv-smartpointer-integration branch June 26, 2024 04:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants