Fixes PointerVector's slicing issue for polymorphic types. #1550

Dimi1010 · 2024-08-25T21:09:08Z

Adds a helper Copier struct that facilitates the copy of polymorphic objects via a clone() member function.

NB: A valid T::clone() method can return either a T* or a std::unique_ptr<T>.

An demo of the changes:

void fn()
{
	PointerVector<int> pvI;
	auto pvI2 = pvI;  // works fine - int is a non-polymorphic type

	struct A
	{
		int y;
	};

	PointerVector<A> pvA1;
	auto pvA2 = pvA1;  // works fine - A is a non-polymorphic type

	struct B
	{
		int y;
		virtual void a() const
		{}

		virtual B* clone() const
		{
			return new B(*this);
		}
	};

	PointerVector<B> pvB1;
	auto pvB2 = pvB1;  // works fine - B is a polymorphic type with a clone() method

	struct C
	{
		int y;
		virtual void a() const
		{}
	};

	PointerVector<C> pvC1;
        auto pvC2 = pvC1;  // compile time error - C is a polymorphic type without a clone method.
}

- Adds a clone method that is enabled when the held type is non-polymorphic or contains a clone method. - Deprecates the copy constructor and copy assignment operator.

codecov · 2024-08-25T21:54:47Z

Codecov Report

Attention: Patch coverage is 0% with 4 lines in your changes missing coverage. Please review.

Project coverage is 82.89%. Comparing base (a600a70) to head (70b9100).
Report is 16 commits behind head on dev.

Files with missing lines	Patch %	Lines
Packet++/src/RawPacket.cpp	0.00%	3 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##              dev    #1550      +/-   ##
==========================================
- Coverage   82.94%   82.89%   -0.05%     
==========================================
  Files         273      273              
  Lines       46353    46265      -88     
  Branches     9642     9387     -255     
==========================================
- Hits        38446    38351      -95     
+ Misses       7280     7057     -223     
- Partials      627      857     +230

Flag	Coverage Δ
fedora39	`74.50% <0.00%> (-0.07%)`	⬇️
macos-12	`80.88% <0.00%> (-0.02%)`	⬇️
macos-13	`80.30% <0.00%> (-0.02%)`	⬇️
macos-14	`80.22% <0.00%> (-0.02%)`	⬇️
mingw32	`70.25% <0.00%> (-1.21%)`	⬇️
mingw64	`70.22% <0.00%> (-1.23%)`	⬇️
npcap	`84.80% <0.00%> (-0.11%)`	⬇️
rhel94	`74.25% <0.00%> (-0.09%)`	⬇️
ubuntu2004	`57.84% <0.00%> (-0.07%)`	⬇️
ubuntu2004-zstd	`57.96% <0.00%> (-0.09%)`	⬇️
ubuntu2204	`74.21% <0.00%> (-0.07%)`	⬇️
ubuntu2204-icpx	`58.39% <0.00%> (-0.08%)`	⬇️
ubuntu2404	`74.44% <0.00%> (-0.09%)`	⬇️
unittest	`82.89% <0.00%> (-0.05%)`	⬇️
windows-2019	`84.86% <0.00%> (-0.09%)`	⬇️
windows-2022	`84.85% <0.00%> (-0.10%)`	⬇️
winpcap	`84.84% <0.00%> (-0.08%)`	⬇️
xdp	`49.20% <0.00%> (-0.10%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Dimi1010 · 2024-08-25T22:03:11Z

Hmm, upon further thoughts, the clone of PointerVector<T> may just be folded into the copy ctor?

Common++/header/PointerVector.h

…y mechanism.

seladb · 2024-08-30T07:34:10Z

Common++/header/PointerVector.h

@@ -343,21 +372,16 @@ namespace pcpp
 		static std::vector<T*> deepCopyUnsafe(std::vector<T*> const& origin)


Maybe we can change the method name to just deepCopy()?

It is specifically named unsafe as the returned vector contains allocated objects that need to be manually freed by the caller. same with freeVectorUnsafe where the pointers in the vector refer to invalid values after the function return.

seladb

One small comment, otherwise LGTM!

seladb · 2024-09-20T07:54:49Z

Packet++/header/RawPacket.h

@@ -1,6 +1,7 @@
 #pragma once

 #include <stdint.h>
+#include <memory>


Why is this include needed? 🤔

Its not. It's left over from when I was prototyping the clone method as i was initially thinking of using unique_ptr but that had issues with unique pointers of Derived types not being covariant of unique pointers to base types.

Packet++/header/RawPacket.h

* Fixes PointerVector's slicing issue for polymorphic types. - Adds a clone method that is enabled when the held type is non-polymorphic or contains a clone method. - Deprecates the copy constructor and copy assignment operator. * Fixed usages of post C++11 symbols. * Updated PointerVector to use Copier helper struct to facilitate a copy mechanism. * Added clone to RawPacket as it is polymorphic. * Remove unused include.

Dimi1010 added 2 commits August 26, 2024 00:06

Fixes PointerVector's slicing issue for polymorphic types.

1291c0c

- Adds a clone method that is enabled when the held type is non-polymorphic or contains a clone method. - Deprecates the copy constructor and copy assignment operator.

Fixed usages of post C++11 symbols.

b134976

Dimi1010 linked an issue Aug 25, 2024 that may be closed by this pull request

PointerVector's copy constructor/assignment can potentially introduce object slicing. #1549

Closed

Dimi1010 requested review from clementperon, seladb, egecetin and tigercosmos August 25, 2024 21:57

Dimi1010 marked this pull request as ready for review August 25, 2024 21:57

seladb reviewed Aug 26, 2024

View reviewed changes

Common++/header/PointerVector.h Show resolved Hide resolved

Dimi1010 marked this pull request as draft August 26, 2024 08:45

Dimi1010 added 3 commits August 29, 2024 21:22

Merge remote-tracking branch 'upstream/dev' into fix/pv-object-slice

5582d48

Updated PointerVector to use Copier helper struct to facilitate a cop…

2826ce5

…y mechanism.

Added clone to RawPacket as it is polymorphic.

f664381

Dimi1010 marked this pull request as ready for review August 29, 2024 19:36

Dimi1010 requested a review from seladb August 29, 2024 19:36

seladb reviewed Aug 30, 2024

View reviewed changes

seladb approved these changes Sep 20, 2024

View reviewed changes

tigercosmos approved these changes Sep 20, 2024

View reviewed changes

Dimi1010 commented Sep 20, 2024

View reviewed changes

Packet++/header/RawPacket.h Outdated Show resolved Hide resolved

Remove unused include.

70b9100

tigercosmos merged commit 1debe6b into seladb:dev Sep 20, 2024
39 checks passed

Dimi1010 deleted the fix/pv-object-slice branch September 20, 2024 19:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes PointerVector's slicing issue for polymorphic types. #1550

Fixes PointerVector's slicing issue for polymorphic types. #1550

Dimi1010 commented Aug 25, 2024 •

edited

Loading

codecov bot commented Aug 25, 2024 •

edited

Loading

Dimi1010 commented Aug 25, 2024 •

edited

Loading

seladb Aug 30, 2024

Dimi1010 Aug 30, 2024 •

edited

Loading

seladb left a comment

seladb Sep 20, 2024

Dimi1010 Sep 20, 2024

		@@ -343,21 +372,16 @@ namespace pcpp
		static std::vector<T> deepCopyUnsafe(std::vector<T> const& origin)

Fixes PointerVector's slicing issue for polymorphic types. #1550

Fixes PointerVector's slicing issue for polymorphic types. #1550

Conversation

Dimi1010 commented Aug 25, 2024 • edited Loading

codecov bot commented Aug 25, 2024 • edited Loading

Codecov Report

Dimi1010 commented Aug 25, 2024 • edited Loading

seladb Aug 30, 2024

Choose a reason for hiding this comment

Dimi1010 Aug 30, 2024 • edited Loading

Choose a reason for hiding this comment

seladb left a comment

Choose a reason for hiding this comment

seladb Sep 20, 2024

Choose a reason for hiding this comment

Dimi1010 Sep 20, 2024

Choose a reason for hiding this comment

Dimi1010 commented Aug 25, 2024 •

edited

Loading

codecov bot commented Aug 25, 2024 •

edited

Loading

Dimi1010 commented Aug 25, 2024 •

edited

Loading

Dimi1010 Aug 30, 2024 •

edited

Loading