Refactor NotificationManager::Notification - #2347
Conversation
|
Build size and comparison to main:
|
|
@mark9064, @NeroBurner Here is refactoring of P.S. InfiniSim doesn't build, because it needs InfiniTimeOrg/InfiniSim#181 |
181276e to
37a9a18
Compare
|
@mark9064 , @NeroBurner I have rebased this PR (and retested services). |
Refactor NotificationManager::Notification to use constructors. This reduces risk of coding errors (incl. buffer overflows) when creating NotificationManager::Notification and copying text to it. And fix a latent bug in ImmediateAlertService (include null terminator in the bytes copied and properly set size) using the constructor.
|
@mark9064 , @NeroBurner Could it be possible to merge this PR (and the corresponding InfiniSim PR InfiniTimeOrg/InfiniSim#181 )? |
mark9064
left a comment
mark9064
left a comment
There was a problem hiding this comment.
Sorry it's been a while. Check the date on that first comment!
| } | ||
|
|
||
| NotificationManager::Notification::Notification(const char* message, uint8_t size) { | ||
| uint8_t effectiveSize = std::min(std::max(size, (uint8_t) 1), NotificationManager::MessageSize); |
There was a problem hiding this comment.
Prefer C++ style static_cast<T>() to C style (T) casting
| NotificationManager::Notification::Notification() { | ||
| } | ||
|
|
||
| NotificationManager::Notification::Notification(const char* message, uint8_t size) { |
There was a problem hiding this comment.
Can you add a docstring to make it clear if the size includes the null pointer or not
| this->size = effectiveSize; | ||
| } | ||
|
|
||
| NotificationManager::Notification::Notification(const struct os_mbuf* om, int off, uint8_t size) { |
3 participants
Refactor NotificationManager::Notification to use constructors. This reduces risk of coding errors (incl. buffer overflows) when creating NotificationManager::Notification and copying text to it.
Additionally fix a latent bug in ImmediateAlertService (include null terminator in the bytes copied and properly set size) using the constructor.
This PR supersedes #2159
This PR needs corresponding refactoring in InfiniSim: InfiniTimeOrg/InfiniSim#181