Re: [PATCH 7/7] xen/device_tree: Fix MISRA C 2012 Rule 20.7 violations

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 26 Aug 2022, Jan Beulich wrote:
> On 26.08.2022 09:58, Xenia Ragiadakou wrote:
> > On 8/26/22 09:21, Jan Beulich wrote:
> >> On 25.08.2022 20:09, Stefano Stabellini wrote:
> >>> But first, let's confirm whether this change:
> >>>
> >>>
> >>>   #define dt_for_each_property_node(dn, pp)                   \
> >>> -    for ( pp = dn->properties; pp != NULL; pp = pp->next )
> >>> +    for ( pp = (dn)->properties; pp != NULL; pp = (pp)->next )
> >>>
> >>>
> >>> is sufficient to make the violation go away in Eclair or cppcheck.  I am
> >>> assuming it is not sufficient, but let's confirm.
> >>
> >> Well, even if for the lhs of assignments there was an exception, this
> >> still wouldn't be sufficient. The minimum needed is
> >>
> >> #define dt_for_each_property_node(dn, pp)                   \
> >>      for ( pp = (dn)->properties; (pp) != NULL; pp = (pp)->next )

Thank you for noticing this


> > If pp is assumed to be a valid lvalue, then why it is needed to add 
> > parentheses here (pp) != NULL ?
> 
> Because in one expression is doesn't matter that in another expression
> the same identifier is used as the lhs of an assignment. Whether
> parentheses are needed should solely depend on the operators in use,
> not any further context.

This is the problem with going with a more sophisticated version of the
rule: it is not immediately obvious any longer. I have to read this
explanation three times to appreciate what it means, I don't think a new
contributor would really have any chances of getting this right,
especially as cppcheck/Eclair wouldn't be able to help them either.