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

[Jan Beulich <jbeulich@xxxxxxxx>]

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@xxxxxxxxxxx>

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.

> --- 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.)

>  /* Prototype:
>   *    struct mcinfo_common *x86_mcinfo_first(struct mc_info *mi);
>   */
> -#define x86_mcinfo_first(_mi)       \
> -    ((struct mcinfo_common *)(_mi)->mi_data)
> +#define x86_mcinfo_first(mi)       \
> +    ((struct mcinfo_common *)(mi)->mi_data)
>  /* Prototype:
>   *    struct mcinfo_common *x86_mcinfo_next(struct mcinfo_common *mic);
>   */
> -#define x86_mcinfo_next(_mic)       \
> -    ((struct mcinfo_common *)((uint8_t *)(_mic) + (_mic)->size))
> +#define x86_mcinfo_next(mic)       \
> +    ((struct mcinfo_common *)((uint8_t *)(mic) + (mic)->size))
> 
>  /* Prototype:
> - *    void x86_mcinfo_lookup(void *ret, struct mc_info *mi, uint16_t type);
> + *    void x86_mcinfo_lookup(void *ret, struct mc_info *mi, uint16_t 
> mc_type);
>   */
> -#define x86_mcinfo_lookup(_ret, _mi, _type)    \
> +#define x86_mcinfo_lookup(ret, mi, mc_type)                     \
>      do {                                                        \
> -        uint32_t found, i;                                      \
> -        struct mcinfo_common *_mic;                             \
> +        uint32_t found_, i_;                                    \
> +        struct mcinfo_common *mic_;                             \
>                                                                  \
> -        found = 0;                                              \
> -        (_ret) = NULL;                                          \
> -        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