MISRA


Nicolas Pitre
 

<rant mode on>

Could someone tell me what is all this MISRA compliance eagerness for?
I can't believe that no one rebelled against that yet.

I just looked at a proposed PR and this makes me want to cry.

Some of the changes are good, like stricter boolean usage.

But the majority of the changes are pure code obfuscation!

Who in its right mind will believe that sprinkling typecasts around
would make the code any safer and understandable? And that's not
mentioning those _double_ typecasts being added too!!! We should instead
aim for cast _reduction_ to let the compiler warn more about type
mismatches and air the code, not add new ones!

Note: Sometimes double casts are necessary, but we do hide them behind
special accessors.

Why do we have to make all loop index vars unsigned? It is a very common
idiom to test for "i < 0" in loops. Making some signed and some unsigned
is just adding to the inconsistency for no gain.

Why do we need to test against an unsigned zero everywhere? Does anyone
know a about a compiler implementation where a signed 0 vs an unsigned
0U makes any difference?

And what is that obsession about qualifying every literal values anyway!
When signed and unsigned expressions are combined, the result is
promoted to unsigned already. So I do ask you frankly which of the
following is the safest and most intuitive:

#define FOO 5
#define BAR 5U

[ ... code far away ... ]

bool foo(int x) { return (x < FOO); }
bool bar(int x) { return (x < BAR); }

Notice how foo(-1) and bar(-1) don't give the same answer. Try it if you
don't believe me.

Whereas:

bool foo(unsigned int x) { return (x < FOO); }
bool bar(unsigned int x) { return (x < BAR); }

Here foo(UINT_MAX) and bar(UINT_MAX) both do give the proper result.

So qualifying literals everywhere when it is not absolutely required is
actually evil and opens the possibility for *more* bugs not less.

And why casting pointer difference to a size_t? The official type for a
pointer difference is ptrdiff_t and the standard offers no explicit
guarantee that sizeof(ptrdiff_t) equals sizeof(size_t). People who live
by the standard will tell you that size_t _could_ be smaller and
therefore silently truncate the result.

What's the point of casting to a char before assigning to a char when
the cast is implicit already?

And a real kicker is this change:

- * @return 0 on success, nonzero on failure
+ * @return false on success, true on failure

Who really thinks that the above is idiomatic and an improvement in code
understandability? It is not that MISRA does require it in this
particular case, but it certainly did skew the mindset of the person who
did that change.

Etc. Etc.

<rant mode off>

So I really do challenge if MISRA compliance is a good thing for the
project. Some of it certainly makes sense. Some of it is really
questionable and should be excluded.

I do guess (even if I don't understand why in the end) that some Zephyr
users do require MISRA compliance for their project. I however do
believe that this is not beneficial for the Zephyr community as a whole,
and that the cost of MISRA compliance and its side effects should not be
imposed on everyone without further scrutiny and discrimination.

Static analysis tools have come a long way and they are far more useful,
realistic and relevant when it comes to code validity and correctness at
this point.

Many embedded libraries and SDK's out there already maintain a MISRA
compliance exception list. I think this would be a sane thing to do and
that wouldn't be a first.


Nicolas

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