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

[nicola <nicola.vetrini@xxxxxxxxxxx>]

On 13/06/23 11:44, Julien Grall wrote:
> 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,
Hi, Julien.

I'm not authorized to send patches about files in the arm32 tree, but 
surely the patch can be easily replicated in any place where it makes 
sense for consistency.
Regards,

  Nicola