Re: [XEN PATCH 01/13] xen/x86: fixed violations of MISRA C:2012 Rule 7.2

[Jan Beulich <jbeulich@xxxxxxxx>]

On 20.06.2023 22:44, Stefano Stabellini wrote:
> On Tue, 20 Jun 2023, Jan Beulich wrote:
>> On 20.06.2023 12:34, 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".
>>>
>>> I propose to use "U" as a suffix to explicitly state when an integer 
>>> constant is represented in an unsigned type.
>>
>> The code adjustments here are certainly fine, but I'd like to ask that
>> patch descriptions be written as such. "I propose ..." in particular
>> may be okay in an upfront discussion, but for a patch you want to
>> describe what the patch does, and why (the latter part you're dealing
>> with already).
>>
>> Furthermore I continue to have trouble with the wording "is represented
>> in an unsigned type": As previously pointed out, what type a constant
>> is going to be represented in depends on the ABI and eventual variables
>> (specifically their types) that the value might then be assigned to, or
>> expressions that the value might be used in. A possible future
>> architecture with "int" wider than 32 bits would represent all the
>> constants touched here in a signed type. I think what is meant instead
>> (despite Misra's imo unhelpful wording) is that you add suffixes for
>> constants which are meant to have unsigned values (no matter what type
>> variable they would be stored in, or what expression they would appear
>> in, and hence independent of their eventual representation).
>>
>> Furthermore the U suffix (as an example) doesn't help at all when the
>> value then is assigned to a variable of type long, and long is wider
>> than int. The value would then _still_ be represented in a signed type.
>>
>> Taken together, how about 'Use "U" as a suffix to explicitly state when
>> an integer constant is intended to be an unsigned one'?
>>
>> I expect both remarks will apply throughout the series, so I'm not
>> going to repeat them for later patches.
> 
> 
> Hi Jan, I agree with you. To further help Gianluca undestand better your
> suggestion, I think the commit message wants to be:
> 
> 
>     xen/x86/acpi/cpufreq: fixed violations of MISRA C:2012 Rule 7.2
> 
>     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".
> 
>     Use "U" as a suffix to explicitly state when an integer constant is
>     intended to be an unsigned one
> 
>     For homogeneity, also add the "U" suffix in other cases that the
>     tool didn't report as violations.
> 
> 
> I also took the opportunity to make the title unique. Jan, if you are
> happy with this wording it could be applied to all patches in this
> series (with the titles being made unique).

Almost. In the case here perhaps: "x86/cpufreq: fix violations of MISRA
C:2012 Rule 7.2". IOW I think subject prefixes shouldn't get too long,
and past tense shouldn't be used unless describing an event in the past.

As a minor further remark, the nested use of double quotes isn't very
nice. When what is to be quoted contains double quotes, I would
typically use single quotes around the construct.

Jan