One of the other developers on my team ran part of our code through SonarQube [1] and well … I have issues with its issues with our C code.
Out of the 600+ issues it flagged, about 500 or so seem to be related to the use of restrict in the code. For example (using my own code):
int btm_cmp(
struct btm const *restrict d1, /* it doesn't like this */
struct btm const *restrict d2 /* or this */
)
{
int rc;
assert(d1 != NULL);
assert(d2 != NULL);
if ((rc = d1->year - d2->year)) return rc;
if ((rc = d1->month - d2->month)) return rc;
if ((rc = d1->day - d2->day)) return rc;
if ((rc = d1->part - d2->part)) return rc;
return 0;
}
I don't care what MISRA (Motor Industry Software Reliability Association) [2] says about it—it signals intent. That these two pointer parameters to the same type are distinct objects and should not be the same object! All you have to see is the function prototypes for the standard functions memcpy() and memmove() to see this. memcpy() is:
extern void *memcpy(void *restrict s1,void const *restrict s2,size_t n);
and thus, the two memory regions aren't supposed to overlap; for memmove():
extern void *memmove(void *s1, void const *s2,size_t n);
the function states the memory regions can overlap. Intent. Geeze.
Of the rest, I don't agree its arbitrary limit of 20 items in a union. The union in question describes packet types of a custom protocol, and I will not split it up just to satisfy some arbitrary limit in the scanning software. Who came up with goto labels being all upper case? No, I don't agree with that as it clashes with over fourty years of C convention where purely uppercase is reserved for constants and macros. I don't agree with the excessive casts it suggests, and I don't agree with removing the one cast I do have.
I disagree about the “useless” parentheses around isdigit because I'm signalling my itent to take the address of the function, not the macro. The C99 standard says this (from section 7.1.4):
Any function declared in a header may be additionally implemented as a function-like macro defined in the header, so if a library function is declared explicitly when its header is included, one of the techniques shown below can be used to ensure the declaration is not affected by such a macro. Any macro definition of a function can be suppressed locally by enclosing the name of the function in parentheses, because the name is then not followed by the left parenthesis that indicates expansion of a macro function name. For the same syntactic reason, it is permitted to take the address of a library function even if it is also defined as a macro.
(emphasis added). The code it's complaining about is:
if (!extract_token(tmp,sizeof(tmp),&p,(isdigit)))
{
/* ... */
}
The various is*() functions are often defined as macros (they are on the compilers we use). I also dislik the “remove useless parentheses” crowd, if only because the C precedence table is screwed up compared to most other languages.
I'm not going to refactor the code just because some scanner thinks the function is doing too much—it's not. Yes, the function itself might be long, but it's converting a rather complex structure from C to Lua (or Lua back to C). I'm not going to break it up just because. Besides, naming things is one of the two hard problems in Computer Science (the others being cache invalidation and off-by-one errors) and I would have to come up with some name for all the new one-use only functions.
But I'm not going to reject everything it said. The suggestions such as reducing scope of variables or making some const are fine. And there were two bugs found, but overall, there was quite a bit of noise to go through. Hopefully, I can argue my case for the ones I disagree with.
We shall see.