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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 06.07.2023 09:57, Juergen Gross wrote:
> 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.

Funny you should say this. Shift counts also aren't allowed to be negative
... For this reason, the pattern I see here is to have U uniformly on the
lhs of the ?: operator, and nowhere else.

Jan