Re: [XEN PATCH 6/6] x86: refactor macros in 'xen-mca.h' to address MISRA C:2012 Rule 5.3

[Nicola Vetrini <nicola.vetrini@bugseng.com>]

On 07/08/2023 10:31, Jan Beulich wrote:
> On 04.08.2023 17:27, Nicola Vetrini wrote:
>> The macros defined 'xen/include/public/arch-x86/xen-mca.h' have 
>> needless
>> underscore prefixes for parameter names and variable names that cause
>> shadowing with e.g. the variable 'i' in function 'mce_action'.
>> Therefore, the renaming aims to resolve present shadowing issues and
>> lessen the probability of future ones.
>> 
>> No functional change.
>> 
>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> 
> I'm okay with the code adjustments here, but I'm afraid I don't follow
> the description: How is shadowing of "i" connected to the use of
> leading underscores in macro parameter names? I think you need to
> separate the two aspects in the wording.
> 

The shadowing remark is tied to the second part of the sentence, but 
I'll try
to make that clearer in v2.

>> --- a/xen/include/public/arch-x86/xen-mca.h
>> +++ b/xen/include/public/arch-x86/xen-mca.h
>> @@ -280,39 +280,39 @@ DEFINE_XEN_GUEST_HANDLE(xen_mc_logical_cpu_t);
>>  /* Prototype:
>>   *    uint32_t x86_mcinfo_nentries(struct mc_info *mi);
>>   */
>> -#define x86_mcinfo_nentries(_mi)    \
>> -    (_mi)->mi_nentries
>> +#define x86_mcinfo_nentries(mi)    \
>> +    (mi)->mi_nentries
> 
> Isn't there another rule demanding parenthization of the whole
> construct? If so, adding the then-missing parentheses right here would
> be quite desirable. (Personally I'm happy about them not being there on
> suffix expressions, as kind of an exception to the general rule.)
> 

If you're referring to Rule 20.7 then it does not require the whole 
expression to be enclosed in
parentheses, as it's concerned with macro parameters (their full 
expansion must parenthesize arguments at
some point, to prevent wrong evaluations due to operator precedence).

>> -        if (_mi == NULL) break;                                 \
>> -        _mic = x86_mcinfo_first(_mi);                           \
>> -        for (i = 0; i < x86_mcinfo_nentries(_mi); i++) {        \
>> -            if (_mic->type == (_type)) {                        \
>> -                found = 1;                                      \
>> +        found_ = 0;                                             \
>> +        (ret) = NULL;                                           \
>> +        if (mi == NULL) break;                                  \
> 
> The lack of parentheses here definitely wants dealing with right away.
> 
> Jan

Good catch

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)