| Message ID | 9d89a58ef016d96da7c3f329fb367f99d169cae6.1691162261.git.nicola.vetrini@bugseng.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen: address MISRA C:2012 Rule 5.3 | expand |
> - _mic = x86_mcinfo_next(_mic); \ > + mic_ = x86_mcinfo_next(mic_); \ > } \ > - (_ret) = found ? _mic : NULL; \ > + (ret) = found_ ? mic_ : NULL; \ > } while (0) > > > -- > 2.34.1 Stray blanks here, sorry.
On Fri, 4 Aug 2023, Nicola Vetrini wrote: > > - _mic = x86_mcinfo_next(_mic); \ > > + mic_ = x86_mcinfo_next(mic_); \ > > } \ > > - (_ret) = found ? _mic : NULL; \ > > + (ret) = found_ ? mic_ : NULL; \ > > } while (0) > > > > > > -- > > 2.34.1 > > Stray blanks here, sorry. Aside from the stray blanks: Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
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. > --- 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
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
diff --git a/xen/include/public/arch-x86/xen-mca.h b/xen/include/public/arch-x86/xen-mca.h index b897536ec5..55b999ab21 100644 --- 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 /* 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; \ + mic_ = x86_mcinfo_first(mi); \ + for (i_ = 0; i_ < x86_mcinfo_nentries(mi); i_++) { \ + if (mic_->type == (mc_type)) { \ + found_ = 1; \ break; \ } \ - _mic = x86_mcinfo_next(_mic); \ + mic_ = x86_mcinfo_next(mic_); \ } \ - (_ret) = found ? _mic : NULL; \ + (ret) = found_ ? mic_ : NULL; \ } while (0)
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> --- The spirit of this patch is similar to this one [1] made by Jan that arose from a violation of this rule. [1] https://gitlab.com/xen-project/xen/-/commit/c0579c65f6bef794cd449fbc946feacccf485f2e --- xen/include/public/arch-x86/xen-mca.h | 38 +++++++++++++-------------- 1 file changed, 19 insertions(+), 19 deletions(-) -- 2.34.1