Re: MISRA
Piotr Pryga
After taking a look at
https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fzephyrproject-rtos%2Fzephyr%2Fpull%2F41227&;data=04%7C01%7Cpiotr.pryga%40nordicsemi.no%7C7ac9d594992e49cf0f5708d9c138a061%7C28e5afa2bf6f419a8cf6b31c6e9e5e8d%7C0%7C0%7C637753272012682798%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=AJcwJotkjxMEWS0bYBIeFvwrlzOvYCS%2B0nfT1TlwbfI%3D&reserved=0 I fully agree with Nicolas and honestly I hate this more than I could have ? possibly imagined before.
The code is now basically a nightmare of explicit typecasts everywhere, and honestly it looks like a nightmare to maintain now.If I may add my 2 cents here... The code looks ugly, but isn't it because the rules were violate in first place and here is fastest (not prettiest) way to make it compliant?
Wouldn't it look good if the types were selected and maintained to avoid casting?
One of the points of MISRA is to make code explicit. If there is no other way than to use cast, then MISRA enforces explicit casting, to make sure an author is aware of what a code does.
In safety critical applications this is mandatory (at least in my humble opinion).
Do not look into past and ugly looking code after fixing compliance issues. Look into future, if there is a way to code to avoid violation of those rules.
It seems to me that it could be achieved....
PIOTR PRYGA | Senior Software Engineer
M +48 507 154 312 | Krakow, Poland
nordicsemi.com | devzone.nordicsemi.com
My question is: is this some kind of exercise that must be done from scratch every release? Because I really doubt that all the PRs in the future can be fully compliant to this typecast madness.Also I understand that the casting is silencing the compiler (and some tool I guess) but has anyone checked whether the code needs some change instead or maybe some boundary / error check? Because typecasting everywhere without investigating why and if some actual code change is needed is just a futile exercise IMHO.
My 2 cents,
Ciao!
PIOTR PRYGA | Senior Software Engineer
M +48 507 154 312 | Krakow, Poland
nordicsemi.com | devzone.nordicsemi.com
-----Original Message-----
From: devel@... <devel@...> On Behalf Of Carlo Caione via lists.zephyrproject.org
Sent: piątek, 17 grudnia 2021 09:38
To: Nicolas Pitre <nico@...>; Gerard Marull Paretas <gerard@...>
Cc: Nashif, Anas <anas.nashif@...>; Olof Johansson <olof@...>; Zephyr Devel <devel@...>
Subject: Re: [Zephyr-devel] MISRA
On 15/12/2021 19:50, Nicolas Pitre wrote:
I persist in insisting that adding any more typecasts to a codebaseAfter taking a look at
makes it worse. Typecasts should be _removed_ as much as possible, not
added. Thinking about how to avoid a cast produces better code almost
all the time. I would hardly trust "compliant" code for safety
critical applications if compliance came about due to added typecasts.
Same thing for qualifying literals as unsigned. I made the
demonstration already showing how evil that is. And no, just saying
that the MISRA scan tool will spot mixed signedness and complain --
that's also missing the point. It is just safer, clearer, lighter and
more enjoyable to just _not_ qualify literals when not required, and
let the compiler promote expressions to unsigned when the
semantic-carrying non-literal part is unsigned, at which point you
don't need a tool to spot mixed signedness because they're just OK.
For those rare cases where mixed signedness is bad there are tricks to
spot them e.g. look at the Linux min() and max() implementations where
the constraint is encapsulated in one place and not spread all over the entire code.
Avoiding so called "undefined" syntax when all existing compiler
implementations handle many of them just fine is stupid. Usually there
is a couple reasons why compilers still implement some "undefined"
behaviors: it is totally logical, uncontroversial, and also useful by
making the code clearer and easier to maintain. Instead of banning
them outright, we should instead implement a test safeguarding our
reliance on them.
https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fzephyrproject-rtos%2Fzephyr%2Fpull%2F41227&;data=04%7C01%7Cpiotr.pryga%40nordicsemi.no%7C7ac9d594992e49cf0f5708d9c138a061%7C28e5afa2bf6f419a8cf6b31c6e9e5e8d%7C0%7C0%7C637753272012682798%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=AJcwJotkjxMEWS0bYBIeFvwrlzOvYCS%2B0nfT1TlwbfI%3D&reserved=0 I fully agree with Nicolas and honestly I hate this more than I could have possibly imagined before.
The code is now basically a nightmare of explicit typecasts everywhere, and honestly it looks like a nightmare to maintain now.
My question is: is this some kind of exercise that must be done from scratch every release? Because I really doubt that all the PRs in the future can be fully compliant to this typecast madness.
Also I understand that the casting is silencing the compiler (and some tool I guess) but has anyone checked whether the code needs some change instead or maybe some boundary / error check? Because typecasting everywhere without investigating why and if some actual code change is needed is just a futile exercise IMHO.
My 2 cents,
Ciao!
--
Carlo Caione