fix wrong cast in morse app #2436
Conversation
zxkmm
commented
Dec 19, 2024
•
zxkmm
commented
- the new cpp syntex from some of the standard (iirc 11) don't need the (void)xxx. just give the type in argument and don't name it
- the original cast for bool is wrong, it would always cast to true
- this fix appear after Morse transmitter loop freeze #2430, the issue there isn't caused by here
* fix wrong cast * sorry, forgot to use enum
| (void)i; // avoid unused warning | ||
| mode_cw = (bool)value; | ||
| options_modulation.on_change = [this](size_t, OptionsField::value_t v) { | ||
| mode_cw = (bool)v; |
There was a problem hiding this comment.
Is there a reason why static_cast doesn't fit the purpose?
There was a problem hiding this comment.
I think static_cast would work too. But imo in this situation, enum is the safest way and best practice for safety and reliability.
Maybe static cast can save some ram usage? Not sure, if you know more info feel free to send a PR and I’ll review.
There was a problem hiding this comment.
In general static_cast is more strict than a regular c-style-cast that can behave as reinterpret_cast in some cases.
There was a problem hiding this comment.
Im not sure if you meant:
- the enum isn't necessary, just static_cast value and give to mode_cw
- we static_cast based on my change instead of c style cast
(bool)
If you meant the first way then im afraid it doesn't make sense, im on phone and didn't take a look of how OptionsField class implemented, but iirc the old usage is wrong, even if you static_cast there, it still will cast to always true.
If you meant the second, I think it's not necessary because it's already protected by enum and no edge case could happen, but if you still want (and have your reason), you and I can change it. afaik all cpp syntax sugar could somehow cost more spaces.
In any case I suggest you to make a PR, and we'll discuss there, instead of email-spam, because if we discuss here, not only me but all the reviewer will be spammed by email in each convo.
