Added documentation comment for protocol type and protocol type family. #1919
Conversation
|
Do tell if there are any suggestions on expanding the section. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #1919 +/- ##
========================================
Coverage 83.47% 83.47%
========================================
Files 298 298
Lines 53950 53950
Branches 11990 11982 -8
========================================
Hits 45034 45034
+ Misses 8242 7705 -537
- Partials 674 1211 +537
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:
|
| /// ProtocolTypeFamily parameter is expected, a single ProtocolType value can be passed instead. The library will | ||
| /// treat it as a family containing only that single protocol. | ||
| /// | ||
| /// @internal |
There was a problem hiding this comment.
nit: I'm not sure this section should be defined as @internal
There was a problem hiding this comment.
I marked it as such, as I think it would be beneficial for those 2 typedefs to be converted to actual structs for type safety eventually.
The current issue with that is their usages in switch statemets. To use a structure in a case XXX: stmt requires the structure to be castable to integral type at compile time. To do this would require all protocol definitions to be constexpr variables, which has some issues when in headers pre-cpp17.
This is why I put that section as internal documentation.
There was a problem hiding this comment.
I don't have a strong opinion, just thought this information could be useful for everyone using this library, not just its developers. If you feel strongly about keeping it, I'm ok with it too 🙂
The PR is ready to merge, so whatever you think...
There was a problem hiding this comment.
I'm gonna keep it internal for now. We can publish more documentation on it if we move to discrete types instead of aliases. As is, the users shouldn't really depend on any internal relations on how it works, as the actual public use cases for ProtocolType and ProtocolTypeFamily are as a more complicated Enum tbh.
| /// @brief The main namespace for the PcapPlusPlus lib | ||
| namespace pcpp | ||
| { | ||
| /// @defgroup ProtocolTypes ProtocolType and ProtocolTypeFamily |
There was a problem hiding this comment.
It would be better if we could do the check using the compiler, but if not, we reviewers need to read the code carefully.
There was a problem hiding this comment.
It would be, but we need cpp17 for that.
# Conflicts: # Packet++/header/ProtocolType.h
In relation to #1916 (comment).
This PR adds a short description on
ProtocolTypeandProtocolTypeFamilyusage in a separate doxygen "topic" page.Note: The paragraphs enclosed inside the
@internaland@endinternalscope, will not generate any documentation unless doxygen is ran withINTERNAL_DOCS=YES. As the section there relates to the internal mechanics and can be subject to change, I decided it is best not to specify it directly in public documentation, to avoid users of the library relying on the information.