Re: MISRA
Nicolas Pitre
On Sat, 18 Dec 2021, Abramo Bagnara wrote:
One thing that could help you get proper reviews is to split your PR in
smaller chunks.
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.
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
Il 17/12/21 23:21, Nicolas Pitre ha scritto:Is that the case? If so I missed it.It does not always have to be 0 means success and <0 means error,IOW you are proposing to do exactly what I presented the first time, but
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).
inverting the boolean logic from original true for failure and false for
success to false for failure and true for success.
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 openYour original change is way too big. Maybe that particular part was OK,
an issue for reverting current logic?
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 thisWell... this is a mess!
case simply avoiding integer and boolean mixing) that changes semantic of
functions would have had *zero* chances to be accepted, am I missing
something?
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