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.vetr...@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.
> --- 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
