Re: [XEN PATCH] xen: fixed violations of MISRA C:2012 Rule 3.1

[Julien Grall <julien@xxxxxxx>]

Hi,

On 13/06/2023 09:27, Jan Beulich wrote:
> On 13.06.2023 09:42, Nicola Vetrini wrote:
> > The xen sources contain several violations of Rule 3.1 from MISRA C:2012,
> > whose headline states:
> > "The character sequences '/*' and '//' shall not be used within a comment".
> >
> > Most of the violations are due to the presence of links to webpages within
> > C-style comment blocks, such as:
> >
> > xen/arch/arm/include/asm/smccc.h:37.1-41.3
> > /*
> >   * This file provides common defines for ARM SMC Calling Convention as
> >   * specified in
> >   * http://infocenter.arm.com/help/topic/com.arm.doc.den0028a/index.html
> >   */
> >
> > In this case, we propose to deviate all of these occurrences with a
> > project deviation to be captured by a tool configuration.
> >
> > There are, however, a few other violations that do not fall under this
> > category, namely:
> >
> > 1. in file "xen/arch/arm/include/asm/arm64/flushtlb.h" we propose to
> > avoid the usage of a nested comment;
> > 2. in file "xen/common/xmalloc_tlsf.c" we propose to substitute the
> > commented-out if statement with a "#if 0 .. #endif;
> > 3. in file "xen/include/xen/atomic.h" and
> > "xen/drivers/passthrough/arm/smmu-v3.c" we propose to split the C-style
> > comment containing the nested comment into two doxygen comments, clearly
> > identifying the second as a code sample. This can then be captured with a
> > project deviation by a tool configuration.
> >
> > Signed-off-by: Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>
> > ---
> > Changes:
> > - Resending the patch with the right maintainers in CC.
> But without otherwise addressing comments already given, afaics. One more
> remark:
>
> > --- a/xen/common/xmalloc_tlsf.c
> > +++ b/xen/common/xmalloc_tlsf.c
> > @@ -140,9 +140,10 @@ static inline void MAPPING_SEARCH(unsigned long *r, int 
> > *fl, int *sl)
> >           *fl = flsl(*r) - 1;
> >           *sl = (*r >> (*fl - MAX_LOG2_SLI)) - MAX_SLI;
> >           *fl -= FLI_OFFSET;
> > -        /*if ((*fl -= FLI_OFFSET) < 0) // FL will be always >0!
> > -         *fl = *sl = 0;
> > -         */
> > +#if 0
> > +        if ((*fl -= FLI_OFFSET) < 0) // FL will be always >0!
> > +        fl = *sl = 0;
> You want to get indentation right here, and you don't want to lose
> the indirection on fl.
>
> > +#endif
> >           *r &= ~t;
> >       }
> >   }
> If you split this to 4 patches, leaving the URL proposal in just
> the cover letter, then I think this one change (with the adjustments)
> could go in right away. Similarly I expect the arm64/flushtlb.h
> change could be ack-ed right away by an Arm maintainer.
I actually dislike the proposal. In this case, the code is meant to look 
like assembly code. I would replace the // with ;. Also, I would like to 
keep the comment style in sync in arm32/flushtlb.h. So can this be 
updated as well?
Cheers,

--
Julien Grall