Re: [PATCH v3 1/2] xen/device_tree: fix Eclair findings for MISRA C 2012 Rule 20.7

[Xenia Ragiadakou <burzalodowa@xxxxxxxxx>]

On 2/8/23 10:38, Julien Grall wrote:
> On 07/02/2023 13:59, Xenia Ragiadakou wrote:
> > Hi Julien,
> Hi Xenia,
>
> > On 2/7/23 15:01, Julien Grall wrote:
> > > Hi,
> > >
> > > On 07/02/2023 12:46, Xenia Ragiadakou wrote:
> > > > On 2/7/23 14:25, Julien Grall wrote:
> > > > > On 07/02/2023 10:46, Xenia Ragiadakou wrote:
> > > > > > On 2/7/23 12:39, Julien Grall wrote:
> > > > > > > Hi,
> > > > > > >
> > > > > > > On 07/02/2023 10:23, Luca Fancellu wrote:
> > > > > > > > > On 3 Feb 2023, at 19:09, Xenia Ragiadakou 
> > > > > > > > > <burzalodowa@xxxxxxxxx> wrote:
> > > > > > > > I’m not really a supporter of empty commit message, but it’s up 
> > > > > > > > to the maintainer :)
> > > > > > > +1. In this case a brief summary of the rule would be handy for 
> > > > > > > those that are not well-versed with MISRA.
> > > > > > > This can be dealt on commit if you propose a new commit message.
> > > > > > I 'm refrained from stating the rule as is because it is strict 
> > > > > > and it is not applied as is.
> > > > > I am a bit confused with this statement. From misra/..., we are 
> > > > > supporting rule 20.7. So why aren't applying it strictly?
> > > > > And if it is not applied as-is, shouldn't we document the violation 
> > > > > (if any)?
> > > > I applied it strictly on v2, but there was no review.
> > > Ah! In general it is best to ping if there are no answers.
> > Me too I ve forgotten about it. A ticket in gitlab reminded me that it 
> > was pending.
> > > > Then Eclair was adjusted to have a less strict approach. Still there 
> > > > is a finding asking to add parentheses around dt in 
> > > > dt_for_each_device_node(dt, dn), i.e dn = (dt);, to which AFAIK you 
> > > > object.
> > > Are you referring to the discussion in [1]? If so, I wasn't totally 
> > > opposed to the suggestion so long we are consistent.
> > I am not able to find [1].
> https://lore.kernel.org/xen-devel/b2f2d1e7-0c18-206f-5e9d-d0115e398840@xxxxxxx/
>
> which is...
>
>
> > I 'm referring to the discussion in 
> > 20221220085100.22848-6-luca.fancellu@xxxxxxx and 
> > 20220728134943.1185621-1-burzalodowa@xxxxxxxxx
> ... a reply to this message. At the end of that reply, I said that I 
> wasn't totally against adding the parentheses but we should be 
> consistent in how we do it.
Ok, I must have got it wrong.

> > > > > > "Add parentheses around macro parameters when the precedence and 
> > > > > > associativity of the performed operators can lead to unintended 
> > > > > > order of evaluation."
> > > > > > Is this ok?
> > > > > I am OK with this. Is there any ID from Eclair that could be used 
> > > > > to track each error (and so we can confirm they have disappeared)?
> > > > I am not aware of any.
> > > Hmmm ok. It might be a nice feature to suggest in Eclair because 
> > > anyone can check whether an issue was resolved.
> > Currently, the violations in Eclair are reported per macro call (I 
> > guess because it is acceptable to have parentheses around the macro 
> > args) so it is difficult to track all of them.
> > > Here, I don't exactly know what to check in Eclair. So I have to rely 
> > > on you. Anyway, nothing that can be fixed for this commit.
> > > > The patch can be decoupled from misra and Eclair (I mean have a 
> > > > generic commit title) and just mention in the commit message that it 
> > > > fixes some Eclair findings for MISRA C rule 20.7.
> > > I have a slight preference for a more generic title. But the current 
> > > one also work for me.
> > It can be changed into "xen/device_tree: add parentheses around macro 
> > parameters"
> > > I will commit later on.
> > Do you want me to send a v4?
> No need. It is now merged with the following commit message:
>
>      xen/device_tree: add parentheses around macro parameters
>
>      Add parentheses around macro parameters when the precedence and
>      associativity of the performed operators can lead to unintended 
> order of evaluation.
>      This is fixing some ECLAIR finding for Misra Rule 20.7.
>
>      Link: 
> https://lore.kernel.org/xen-devel/20230203190908.211541-2-burzalodowa@xxxxxxxxx/
>      Signed-off-by: Xenia Ragiadakou <burzalodowa@xxxxxxxxx>
>      Reviewed-by: Luca Fancellu <luca.fancellu@xxxxxxx>
>      [jgrall: Reworded the commit message]
>      Acked-by: Julien Grall <jgrall@xxxxxxxxxx>
Thanks a lot.

> Cheers,
--
Xenia