Re: MISRA


Nicolas Pitre
 

On Sat, 18 Dec 2021, Abramo Bagnara wrote:

Il 17/12/21 23:21, Nicolas Pitre ha scritto:

It does not always have to be 0 means success and <0 means error,
especially in this case where Z_SYSCALL_VERIFY_MSG() has a clear
semantic i.e. it is mainly testing a condition and does not perform any
substantial operation (the log is merely a side effect).
IOW you are proposing to do exactly what I presented the first time, but
inverting the boolean logic from original true for failure and false for
success to false for failure and true for success.
Is that the case? If so I missed it.

One thing that could help you get proper reviews is to split your PR in
smaller chunks.

It sounds great, but wasn't better then to accept my original changes and open
an issue for reverting current logic?
Your original change is way too big. Maybe that particular part was OK,
but it was lumped with many other things and this is not conducive to
quality reviews.

In particular, you should split more or less along different themes:

* a PR for boolean:

- use bool when the data nature is Boolean
- use explicit comparison with 0 or NULL

* another PR for enums:

- avoid mixing enumerations with integers

Etc.

This way, the uncontrovertial parts can be reviewed and merged quickly,
while the other parts can be judged on their own merit separately.

In particular, I think that the following are pointless and harmful to
code clarity:

- added U suffix to unsigned constants
- avoid mixing signed and unsigned integers in computations and
comparisons (should be OK if mixedness comes from unqualified iterals)
- avoid mixing characters with integers;

And again, any added cast to oobtain compliance is harmful to code
quality and must be avoided if alternatives are possible.

It seems obvious to me that a change aimed to improve code quality (in this
case simply avoiding integer and boolean mixing) that changes semantic of
functions would have had *zero* chances to be accepted, am I missing
something?
Well... this is a mess!

Even the comment for Z_SYSCALL_VERIFY_MSG() is garbage. It says that an
oops is triggered if the tested expression is false which is wrong as
there is no oops triggering at all in the current code. Bad bad bad.


Nicolas

Join devel@lists.zephyrproject.org to automatically receive all group messages.