Refactor NotificationManager::Notification - #2347
Conversation
|
Build checks have not completed. Possible reasons for this are:
|
|
@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). |
|
@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!
|
@mark9064 Sorry - I had somehow missed your comment about the cast style! Now it should be fixed in both places. |
mark9064
left a comment
mark9064
left a comment
There was a problem hiding this comment.
Haha sorry - the comment on the cast wasn't published, I wrote it a long time ago but never completed the review for some reason :)
| * @param size the size of complete message, including the final 0x00 byte | ||
| */ | ||
| NotificationManager::Notification::Notification(const char* message, uint8_t size) { | ||
| uint8_t effectiveSize = std::min(std::max(size, static_cast<uint8_t>(1)), NotificationManager::MessageSize); |
There was a problem hiding this comment.
I'm curious if this could be just 1U, not sure if that would work. Probably not
There was a problem hiding this comment.
Unfortunately neither 1u nor 1U worked here. 🙁
There was a problem hiding this comment.
uniform initialization could work and is a bit shorter uint_8t{1}. It is a form I'm not so used to use, and I don't know if it is "good" C++ 😅
There was a problem hiding this comment.
I just had a look at this at this might be the best way - the argument supposedly is with uniform initialisation it's impossible to initialise with an invalid value, whereas this is possible with a cast
There was a problem hiding this comment.
Thanks, @NeroBurner!
I have updated the PR to use uniform initialization and rebased.
P.S. @mark9064 Could the build-firmware be failing due to recent GCC and linking changes?
1a38d3c to
1eef235
Compare
Refactor NotificationManager::Notification to use constructors. This reduces risk of coding errors (incl. buffer overflows) when creating NotificationManager::Notification and copying text to it.
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