LoupVaillant
commented
4 years ago Hi Thomas,
I wouldn't mind making the code a few lines longer, but for marketing reasons the buck stops at 1999 lines of code (as counted by sloccount, in monocypher.c only). Ideally 1989 lines of code, so we can honestly say "less than 2K lines of code". The current count is 1985 lines, that's not a lot of room.
You probably guessed, but the reason for u8, u32 etc. in monocypher.c is brevity and clarity. Shorter names work better when they're used all over the place. Using the longer type names for the prototypes in the source file would trade one inconsistency for another: sure, declaration and definition would match, but then we'd have a mix of short & long type names in the source code, which might be a bit jarring. For this reason, I believe I disagree with MISRA on this one.
I never thought twice about using signed (but positive) literals with unsigned variables. I wouldn't mind suffixing an "u" if it makes the tools happy, though. Any other implicit conversion should definitely be made explicit. Not just signed/unsigned, but any conversion that reduces precision. I've removed most of them thanks to Clang/MSVC warnings, but others may linger.
I believe MISRA hates macros, but I'm definitely not going to conform there. :-) If however it has something to say about how macros are defined, I could be persuaded to address its warnings if it doesn't hurt readability too much.
Any other warning may be addressed (or denied) on a case by case basis. One thing we can definitely do is put a comment block explaining which MISRA rule are infringed, and why. This could help users (i) suppress warnings when analysing monocypher.c, (ii) explain the rationale behind each non-compliance, and (iii) signal that there is a rationale to begin with.
skoe
Hi Loup,
There are various constructs in C that are usually totally fine when used correctly but may be dangerous on corner cases or when used incorrectly. That's why some coding style guides forbid them, one of them is MISRA.
I'm aware that you use various code analysis tools and don't suspect that there are bugs lurking. But I wanted to ask what you think about ultra defensive code style and whether you would accept pull requests which address some of these issues. Maybe it would further increase the trustworthiness of the code base and make it easier to integrate it into some projects with stricter code style rules. But on the other hand it would make the source code more verbose. Some examples:
monocypher.c redefines stdint.h types to u8, u32 etc.. monocypher.h uses the stdint.h types directly. This causes the implementation of a function to use u8 while the prototype declares uint8_t for the same parameter. Some code analysis tools complain about it (not MISRA conform), even though they refer to the same type indirectly. It could have advantages to use the stdint.h types directly everywhere.
Often signed and unsigned arithmetic is mixed and/or signed literals are used together with unsigned variables. The code is mostly correct, but not all programmers actually understand all rules that apply for implicit signed/unsigned conversions especially in complex terms. That's why it may be more defensive to mark literals with the "U" suffix when they are used in unsigned context.
What do you think? What is your strategy here?