| Message ID | 5aa3a54af456b8faee681a1d737c361abe89296f.1687250177.git.gianluca.luparini@bugseng.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen: fixed violations of MISRA C:2012 Rule 7.2 | expand |
On 20.06.2023 12:34, 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". > > 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. Jan
On Tue, 20 Jun 2023, Jan Beulich wrote: > On 20.06.2023 12:34, 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". > > > > 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).
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@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". >>> >>> 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
diff --git a/xen/arch/x86/acpi/cpufreq/powernow.c b/xen/arch/x86/acpi/cpufreq/powernow.c index d4c7dcd5d9..8e0784b69c 100644 --- a/xen/arch/x86/acpi/cpufreq/powernow.c +++ b/xen/arch/x86/acpi/cpufreq/powernow.c @@ -32,14 +32,14 @@ #include <acpi/acpi.h> #include <acpi/cpufreq/cpufreq.h> -#define HW_PSTATE_MASK 0x00000007 -#define HW_PSTATE_VALID_MASK 0x80000000 -#define HW_PSTATE_MAX_MASK 0x000000f0 +#define HW_PSTATE_MASK 0x00000007U +#define HW_PSTATE_VALID_MASK 0x80000000U +#define HW_PSTATE_MAX_MASK 0x000000f0U #define HW_PSTATE_MAX_SHIFT 4 -#define MSR_PSTATE_DEF_BASE 0xc0010064 /* base of Pstate MSRs */ -#define MSR_PSTATE_STATUS 0xc0010063 /* Pstate Status MSR */ -#define MSR_PSTATE_CTRL 0xc0010062 /* Pstate control MSR */ -#define MSR_PSTATE_CUR_LIMIT 0xc0010061 /* pstate current limit MSR */ +#define MSR_PSTATE_DEF_BASE 0xc0010064U /* base of Pstate MSRs */ +#define MSR_PSTATE_STATUS 0xc0010063U /* Pstate Status MSR */ +#define MSR_PSTATE_CTRL 0xc0010062U /* Pstate control MSR */ +#define MSR_PSTATE_CUR_LIMIT 0xc0010061U /* pstate current limit MSR */ #define MSR_HWCR_CPBDIS_MASK 0x02000000ULL #define ARCH_CPU_FLAG_RESUME 1 diff --git a/xen/include/acpi/cpufreq/processor_perf.h b/xen/include/acpi/cpufreq/processor_perf.h index d8a1ba68a6..8b5a1b9bde 100644 --- a/xen/include/acpi/cpufreq/processor_perf.h +++ b/xen/include/acpi/cpufreq/processor_perf.h @@ -5,7 +5,7 @@ #include <public/sysctl.h> #include <xen/acpi.h> -#define XEN_PX_INIT 0x80000000 +#define XEN_PX_INIT 0x80000000U int powernow_cpufreq_init(void); unsigned int powernow_register_driver(void);