Re: [PATCH 2/8] pnv/psi: Clean up local variable shadowing

[Cédric Le Goater]

On 9/29/23 07:30, Markus Armbruster wrote:
> Cédric Le Goater <clg@kaod.org> writes:
>
> > On 9/19/23 08:57, Harsh Prateek Bora wrote:
> > > On 9/18/23 20:28, Cédric Le Goater wrote:
> > > > to fix :
> > > >
> > > >     ../hw/ppc/pnv_psi.c: In function ‘pnv_psi_p9_mmio_write’:
> > > >     ../hw/ppc/pnv_psi.c:741:24: warning: declaration of ‘addr’ shadows a 
> > > > parameter [-Wshadow=compatible-local]
> > > >       741 |                 hwaddr addr = val & ~(PSIHB9_ESB_CI_VALID | 
> > > > PSIHB10_ESB_CI_64K);
> > > >           |                        ^~~~
> > > >     ../hw/ppc/pnv_psi.c:702:56: note: shadowed declaration is here
> > > >       702 | static void pnv_psi_p9_mmio_write(void *opaque, hwaddr addr,
> > > >           |                                                 ~~~~~~~^~~~
> > > >
> > > > Signed-off-by: Cédric Le Goater <clg@kaod.org>
> > > > ---
> > > >    hw/ppc/pnv_psi.c | 5 +++--
> > > >    1 file changed, 3 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/hw/ppc/pnv_psi.c b/hw/ppc/pnv_psi.c
> > > > index daaa2f0575fd..26460d210deb 100644
> > > > --- a/hw/ppc/pnv_psi.c
> > > > +++ b/hw/ppc/pnv_psi.c
> > > > @@ -738,8 +738,9 @@ static void pnv_psi_p9_mmio_write(void *opaque, hwaddr addr,
> > > >                }
> > > >            } else {
> > > >                if (!(psi->regs[reg] & PSIHB9_ESB_CI_VALID)) {
> > > > -                hwaddr addr = val & ~(PSIHB9_ESB_CI_VALID | 
> > > > PSIHB10_ESB_CI_64K);
> > > > -                memory_region_add_subregion(sysmem, addr,
> > > > +                hwaddr esb_addr =
> > >
> > > While at it, we may want to move the declaration to the beginning of the 
> > > function.
> >
> > I am more in favor of declaring the variables where they are needed.
> > I think it is better pratice since it identifies a functional block
> > which could be move in a external routine at some point if it becomes
> > too complex.
>
> I'm old-fashioned and prefer my declarations in one place, where I can
> find them easily, except perhaps for for-loop counters.  

This is also a good practice since too much local variables in a
routine is a good sign of further splitting need. Anyway as you
said, we have -Wshadow=local, we should be safe.

Thanks,

C.



> I suspect when
> a function is big enough so that moving declarations to inner blocks
> improves it, factoring out these inner blocks would improve it more.
>
> My other reason is the risk for accidental shadowing, but us enabling
> -Wshadow=local will render that point moot.
>
> Maintainers get to decide such matters of taste, and I'm not the
> maintainer here :)
>
> > > Anyways,
> > > Reviewed-by: Harsh Prateek Bora <harshpb@linux.ibm.com>
> >
> >
> > Thanks,
> >
> > C.
> >
> >
> > > > +                    val & ~(PSIHB9_ESB_CI_VALID | PSIHB10_ESB_CI_64K);
> > > > +                memory_region_add_subregion(sysmem, esb_addr,
> > > >                                                &psi9->source.esb_mmio);
> > > >                }
> > > >            }