Il 17/12/21 09:37, Carlo Caione ha scritto:

On 15/12/2021 19:50, Nicolas Pitre wrote:

I persist in insisting that adding any more typecasts to a codebase
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.

After taking a look at https://github.com/zephyrproject-rtos/zephyr/pull/41227 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.

Let me talk clear: in the portion of Zephyr currently checked we have more than 18K violations of what I think is one of the most valuable rule of BARR C that asks that any non obviously value-preserving cast is accompanied by a comment that clarify why such value change does not happen or why such value change is deliberate.

My PR has shown some important points where implicit casts happens and that need to be explicited to improve readability/understanding of code so to improve code quality *and* to increase MISRA compliance.

My request to code owners/PR reviewers is to question such points and use this info for one of:

- (possibly implicit) confirmation that the cast (now observable) is ok (either because it is value preserving or because the value change is deliberate). If the code owner is willing to add or suggest a comment it is welcome.

- use the added cast as a way to understand that code has *indeed* an existing problem that should be fixed and that this PR fortunately has made evident.

Also in some points I've added /*? shaped comments to points out that the code sounds very suspect to my eyes (possibly simply because I have not realized the intention).

IMHO the point to understand is that this is a path to improvement: the aim is to reduce the number of points where unwanted value change happens, for code safety sake.


--
Abramo Bagnara

BUGSENG srl - http://bugseng.com
mailto:abramo.bagnara@...