Add enum for RawPacket concrete implementation.#2062
Add enum for RawPacket concrete implementation.#2062Dimi1010 wants to merge 8 commits intoseladb:devfrom
Conversation
Dimi1010
added
refactoring
API deprecation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #2062 +/- ##
==========================================
- Coverage 83.81% 83.80% -0.01%
==========================================
Files 313 313
Lines 55676 55673 -3
Branches 11606 11578 -28
==========================================
- Hits 46665 46659 -6
- Misses 7810 7826 +16
+ Partials 1201 1188 -13
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:
|
| enum class RawPacketImplType : uint8_t | ||
| { | ||
| /// @brief Unknown type | ||
| Unknown = 0, |
There was a problem hiding this comment.
It is so zero initialized values have a separate enum value that won't just be assumed to be a certain implementation.
It is mostly a defensive programming technique.
There was a problem hiding this comment.
Since the base class RawPacket is the default, I think it should be the default for the enum as well
There was a problem hiding this comment.
The RawPacket being nominal default implementation doesn't guarantee it is to be assumed everywhere tho?
I would prefer not to risk it in this case, as it can lead to memory corruption bugs due to improper down casting and those are hard to track down. It is done this way precisely to reduce general assumptions, which can be dangerous when working with memory.
|
@Dimi1010 do you know why the "Generate Cobertura Report" phase randomly fails? It uses |
Tbh, I'm not really sure either. I can try to debug it, but I'm not promising anything. |
@seladb created a separate issue #2067 for that so it isn't scattered in PRs. |
seladb
left a comment
seladb
left a comment
There was a problem hiding this comment.
Please see the comment below, otherwise LGTM
| enum class RawPacketImplType : uint8_t | ||
| { | ||
| /// @brief Unknown type | ||
| Unknown = 0, |
There was a problem hiding this comment.
Since the base class RawPacket is the default, I think it should be the default for the enum as well
2 participants
This PR replaces the raw packet type identifier integers
uint8_twith strongly typed enuminternal::RawPacketImplType.The associated method
RawPacket::getObjectType()has been deprecated in favor ofRawPacket::getImplType().The new method
getImplTypehas been documented as for internal use, and should not be considered part of the stable API.