Re: [XEN PATCH v2 09/13] xen/public: fix violations of MISRA C:2012 Rule 7.2

[Juergen Gross <jgross@xxxxxxxx>]

On 06.07.23 09:43, Jan Beulich wrote:
> On 05.07.2023 17:33, Juergen Gross wrote:
> > On 05.07.23 17:26, Simone Ballarin wrote:
> > > From: Gianluca Luparini <gianluca.luparini@xxxxxxxxxxx>
> > >
> > > The xen sources contains violations of MISRA C:2012 Rule 7.2 whose
> > > headline states:
> > > "A 'u' or 'U' suffix shall be applied to all integer constants
> > > that are represented in an unsigned type".
> > >
> > > Add the 'U' suffix to integers literals with unsigned type and also to other
> > > literals used in the same contexts or near violations, when their positive
> > > nature is immediately clear. The latter changes are done for the sake of
> > > uniformity.
> > >
> > > Signed-off-by: Simone Ballarin <simone.ballarin@xxxxxxxxxxx>
> > > Signed-off-by: Gianluca Luparini <gianluca.luparini@xxxxxxxxxxx>
> > > ---
> > > Changes in v2:
> > > - minor change to commit title
> > > - change commit message
> > > - correct macros code style
> > > ---
> > >    xen/include/public/io/ring.h | 10 +++++-----
> > >    1 file changed, 5 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/xen/include/public/io/ring.h b/xen/include/public/io/ring.h
> > > index 025939278b..0cae4367be 100644
> > > --- a/xen/include/public/io/ring.h
> > > +++ b/xen/include/public/io/ring.h
> > > @@ -36,11 +36,11 @@
> > >    typedef unsigned int RING_IDX;
> > >    
> > >    /* Round a 32-bit unsigned constant down to the nearest power of two. */
> > > -#define __RD2(_x)  (((_x) & 0x00000002) ? 0x2                  : ((_x) & 0x1))
> > > -#define __RD4(_x)  (((_x) & 0x0000000c) ? __RD2((_x)>>2)<<2    : __RD2(_x))
> > > -#define __RD8(_x)  (((_x) & 0x000000f0) ? __RD4((_x)>>4)<<4    : __RD4(_x))
> > > -#define __RD16(_x) (((_x) & 0x0000ff00) ? __RD8((_x)>>8)<<8    : __RD8(_x))
> > > -#define __RD32(_x) (((_x) & 0xffff0000) ? __RD16((_x)>>16)<<16 : __RD16(_x))
> > > +#define __RD2(x)  (((x) & 0x00000002U) ? 0x2                     : ((x) & 0x1))
> > Shouldn't this be rather:
> >
> > +#define __RD2(x)  (((x) & 0x00000002U) ? 0x2U                   : ((x) & 0x1U))
> I don't think it matters much (as the comment says, the input is expected
> to be unsigned anyway), and I expect even the one U that was added here
> was only added for consistency. The sole one that really matter is imo ...
>
> > > +#define __RD4(x)  (((x) & 0x0000000cU) ? __RD2((x) >> 2) << 2    : __RD2(x))
> > > +#define __RD8(x)  (((x) & 0x000000f0U) ? __RD4((x) >> 4) << 4    : __RD4(x))
> > > +#define __RD16(x) (((x) & 0x0000ff00U) ? __RD8((x) >> 8) << 8    : __RD8(x))
> > > +#define __RD32(x) (((x) & 0xffff0000U) ? __RD16((x) >> 16) << 16 : __RD16(x))
> ... this single one.
I agree that only the last one is really needed.

But for consistency reasons I'd expect all optional "U"s to be either dropped or
specified, instead of a mixture.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature