On 06.07.23 10:43, Jan Beulich wrote:
> 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@bugseng.com>
>>>>>
>>>>> 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@bugseng.com>
>>>>> Signed-off-by: Gianluca Luparini <gianluca.luparini@bugseng.com>
>>>>> ---
>>>>> 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.

Yes, this is one way to look at it.

My view would be that (at least) the constants used for ANDing should have a
uniform U attribution.

In the end I'm fine with either way. I just wanted to point out a slight
inconsistency with the patch.


Juergen