Re: MISRA C Rule 20.7 disambiguation

[Jan Beulich <jbeulich@xxxxxxxx>]

On 09.12.2022 01:45, Stefano Stabellini wrote:
> This patch is to start a discussion in regard to rule 20.7 and its
> interpretation. During the last MISRA C call we discussed that "our"> 
> interpretation of the rule means that the following two cases don't need
> extra parenthesis:
> 
> #define M(a, b) func(a, b)
> #define M(a, b) (a) = b

I'm puzzled by the latter. Iirc there was discussion on whether the LHS
of an assignment needs parentheses, but I don't think there was any
question about the RHS wanting them, irrespective of the facts that only
comma expressions have lower precedence than assignment ones and that
evaluation goes right to left anyway.

One aspect speaking for parentheses even on the LHS is that an expression
(rather than an lvalue) passed as macro argument then uniformly becomes
invalid, i.e. not just

        M(x + y, z)

would be rejected by the compiler, but also

        M(x = y, z)

.

> Moreover, MISRA C states that parenthesis should be added when the
> expansion of a MACRO parameters would result in an *expression*.
> 
> Expression is the important word. Looking at this *compliant* example
> from the manual:
> 
> #define GET_MEMBER( S, M ) ( S ).M
> 
> It is compliant because S results in an expression so it needs
> parenthesis, while M does not, so it doesn't need parenthesis.
> 
> My understanding is the following:
> - is it possible to pass an expression as a parameter of the MACRO?
>     - if yes -> need parenthesis
>     - if no  -> doesn't need parenthesis
> 
> 
> As an example, cppcheck reports the following (from xmalloc.h) as
> violation:
> 
> #define xmalloc_array(_type, _num) \
>     ((_type *)_xmalloc_array(sizeof(_type), __alignof__(_type), _num))
> 
> I think this is a false positive. We have already enstablished that the
> "," operator doesn't require parenthesis, so "_num" is not the problem.
> And there is no way that adding parenthesis to "type" would allow an
> expression to be passed as the type argument.

"Allow" (here and elsewhere) is probably not a good word. You can pass
_anything_ to a macro. The question is whether the macro expansion
would yield something sensible. And another question is whether with
parentheses added the result actually still compiles when the macro is
used as intended. The 2nd aspect is relevant here - you cannot add
parentheses like this: ((_type)*) - this isn't a well formed cast
anymore, and the compiler will complain. _If_ this is what cppcheck is
complaining about, then this imo is a pretty clear bug in the tool.

> Let's take another example:
> 
> #define xzalloc_flex_struct(type, field, nr) \
>     ((type *)_xzalloc(offsetof(type, field[nr]), __alignof__(type)))
> 
> "type" is the same as last time. There are 2 other interesting macro
> parameters here: nr and field.
> 
> nr could result in an expression, but I don't think it needs
> parenthesis because it is between []? However, we know we have a clear
> exception for the "," operator. We don't have a clear exception for the
> [] operator. Do we need (nr)?

The question of whether parentheses are needed clearly need to be based
on whether without parentheses anything that looks sensible on the surface
can be mistaken for other than what was meant. I think [] simply are
another form of parenthesization, even if these are commonly called square
bracket (not parentheses). For this to be mistaken, a macro argument would
need to be passed which first has a ] and then a [. This clearly doesn't
look sensible even when just very briefly looking at it. Plus the same
issue would exist with parentheses: You could also undermine the proper
use of parentheses in the macro by passing a macro argument which first
has ) and then (. IOW - adding parentheses here adds no value, and hence
is merely clutter.

> field could result in an expression, so I think it needs parenthesis.

No, field (and intentionally named that way) is a struct member indicator.
Neither p->(field) nor s.(field) are syntactically valid. There simply
cannot be parentheses here, so the same conclusion as near the top applies.

> Just to be clear, I'll list all the possible options below.
> 
> a) no changes needed, xzalloc_flex_struct is good as is

This is it, and not surprisingly: This construct was introduced not that
long ago, when we already paid close attention to parenthesization needs.

Jan

> b) only "field" needs parenthesis
> c) only "nr" needs parenthesis
> d) both "field" and "nr" need parenthesis
> 
> Option d) would look like this:
> 
> #define xzalloc_flex_struct(type, field, nr) \
>     ((type *)_xzalloc(offsetof(type, (field)[(nr)]), __alignof__(type)))
> 
> What do you guys think?
> 
> Cheers,
> 
> Stefano