Re: [PATCH 5/8] spapr: Clean up local variable shadowing in spapr_get_fw_dev_path()

[Cédric Le Goater]

On 9/19/23 10:23, Harsh Prateek Bora wrote:
> On 9/18/23 20:28, Cédric Le Goater wrote:
> > Rename PCIDevice variable to avoid this warning :
> >
> >    ../hw/ppc/spapr.c: In function ‘spapr_get_fw_dev_path’:
> >    ../hw/ppc/spapr.c:3217:20: warning: declaration of ‘pcidev’ shadows a 
> > previous local [-Wshadow=compatible-local]
> >     3217 |         PCIDevice *pcidev = CAST(PCIDevice, dev, TYPE_PCI_DEVICE);
> >          |                    ^~~~~~
> >    ../hw/ppc/spapr.c:3147:16: note: shadowed declaration is here
> >     3147 |     PCIDevice *pcidev = CAST(PCIDevice, dev, TYPE_PCI_DEVICE);
> >          |                ^~~~~~
> >
> > Signed-off-by: Cédric Le Goater <clg@kaod.org>
> > ---
> >   hw/ppc/spapr.c | 4 ++--
> >   1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 41ce7de77c14..8090fb0302df 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -3214,8 +3214,8 @@ static char *spapr_get_fw_dev_path(FWPathProvider *p, 
> > BusState *bus,
> >       if (g_str_equal("pci-bridge", qdev_fw_name(dev))) {
> >           /* SLOF uses "pci" instead of "pci-bridge" for PCI bridges */
> > -        PCIDevice *pcidev = CAST(PCIDevice, dev, TYPE_PCI_DEVICE);
> > -        return g_strdup_printf("pci@%x", PCI_SLOT(pcidev->devfn));
> > +        PCIDevice *pdev = CAST(PCIDevice, dev, TYPE_PCI_DEVICE);
> > +        return g_strdup_printf("pci@%x", PCI_SLOT(pdev->devfn));
>
> Instead of renaming, can't we use pcidev itself without re-declaring ?

I agree this could be done. But there is this test below :
 
> >       }
> >       if (pcidev) {

which makes use of the same variable. I would rather keep the same
logic in the code or rewrite the routine completely with something
like:

    if (object_dynamic_cast(OBJECT(dev), TYPE_1 {
        /* return this */
    } else if (object_dynamic_cast(OBJECT(dev), TYPE_2 {
        /* or that */
    } else if (object_dynamic_cast(OBJECT(dev), "some string"))
    ....
    } else {
        error_report("...");
    }

    return NULL;


This is beyond the issue that this patch is trying to fix.

Thanks,

C.