Refactored PcapHandle to utilize std::unique_ptr internally. #1753
Conversation
There was a problem hiding this comment.
Should we have an /internal/ folder for headers that need to be public for compilation but shouldn't be included by users directly?
There was a problem hiding this comment.
I am okay with the current way, but it's also fine to move the files.
There was a problem hiding this comment.
It feels cleaner to have the "internal" public headers separated. That way, we don't have to worry about breaking changes if "internal" files are renamed or moved around due to people directly including them. And if they still do, that is on them.
There was a problem hiding this comment.
Well, I agree with you. Just think we can put effort on more important parts (if there are such things)
There was a problem hiding this comment.
Eh, it's possibly not the most important. But since this PR exposes a new public header (PcapUtils.h), that only defines internal symbols, I would prefer to have that decided now instead of maybe having to do a transition later, when it has already been placed next to the user API headers.
There was a problem hiding this comment.
Got it. I don't have strong opion on this. Let see how @seladb says.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## dev #1753 +/- ##
==========================================
- Coverage 83.21% 83.20% -0.01%
==========================================
Files 282 282
Lines 48719 48701 -18
Branches 10547 10542 -5
==========================================
- Hits 40540 40523 -17
- Misses 7052 7053 +1
+ Partials 1127 1125 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| @@ -34,6 +34,7 @@ set( | |||
| header/PcapFilter.h | |||
| header/PcapLiveDevice.h | |||
| header/PcapLiveDeviceList.h | |||
| header/PcapUtils.h | |||
There was a problem hiding this comment.
I think we should avoid exposing internal header files like PcapUtils.h.
The reason it is needed is because we use internal::PcapCloseDeleter inside a "public" header file, and we should find a way around it.
I tried using forward declaration of internal::PcapCloseDeleter but couldn't make it work so far. @Dimi1010 maybe you can make it work?
Other options are:
- Move
internal::PcapCloseDeletertoPcapDevice.h- not ideal, but I think it'll work - Define
m_PcapDescriptoras "no type" likevoid*and assign the type in the.cppfile. However it's a pretty ugly workaround
There was a problem hiding this comment.
See this as a potential solution. I think this is the cleanest, tbh.
#1753 (comment)
internal::PcapCloseDeleter can't work as a forward declaration here as it is default constructed in the std::unique_ptr so it needs a concrete type.
There was a problem hiding this comment.
I think exposing an internal folder of headers is not a good solution either because exposing header files just because we have some technical limitations is not something we want. I think we should either find a workaround (even if it's not very nice), or just leave the code as it is
There was a problem hiding this comment.
I have seen that pattern be used to separate the stable public headers from the unstable ones, that can change at any moment and users shouldn't rely upon. Most notably I have seen it in many boost libraries ("details" folders).
Moving internal::PcapCloseDeleter to PcapDevice.h creates a dependency between PcapFilter.cpp and PcapDevice.h, which I would prefer to not have.
I guess we can leave this as is for now tho.
There was a problem hiding this comment.
If it was "stable" vs "unstable" public API I'd agree that separating them into different folders could work. But exposing a "private API" (internal code) is probably not the best way to go.
I guess leaving it as is is fine if we can find a better solution...
No description provided.