[PATCH 0/4] aspeed: Clean up local variable shadowing

[PATCH 0/4] aspeed: Clean up local variable shadowing

[Cédric Le Goater]

Hello,

Here are cleanups for local variable shadowing warnings in aspeed models.

Joel, Andrew,

Could you please double check patch 4 ? 

Thanks,

C. 

Cédric Le Goater (4):
  aspeed/i2c: Clean up local variable shadowing
  aspeed: Clean up local variable shadowing
  aspeed/i3c: Rename variable shadowing a local
  aspeed/timer: Clean up local variable shadowing

 hw/arm/aspeed_ast2600.c | 10 +++++-----
 hw/i2c/aspeed_i2c.c     |  1 -
 hw/misc/aspeed_i3c.c    |  6 +++---
 hw/timer/aspeed_timer.c |  2 +-
 4 files changed, 9 insertions(+), 10 deletions(-)

-- 
2.41.0

[PATCH 1/4] aspeed/i2c: Clean up local variable shadowing

[Cédric Le Goater]

Remove superfluous local 'data' variable and use the one define at the
top of the routine. This fixes :

  ../hw/i2c/aspeed_i2c.c: In function ‘aspeed_i2c_bus_recv’:
  ../hw/i2c/aspeed_i2c.c:315:17: warning: declaration of ‘data’ shadows a previous local [-Wshadow=compatible-local]
    315 |         uint8_t data;
        |                 ^~~~
  ../hw/i2c/aspeed_i2c.c:288:13: note: shadowed declaration is here
    288 |     uint8_t data;
        |             ^~~~

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/i2c/aspeed_i2c.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
index 7275d40749a9..1037c22b2f79 100644
--- a/hw/i2c/aspeed_i2c.c
+++ b/hw/i2c/aspeed_i2c.c
@@ -312,7 +312,6 @@ static void aspeed_i2c_bus_recv(AspeedI2CBus *bus)
         SHARED_ARRAY_FIELD_DP32(bus->regs, reg_pool_ctrl, RX_COUNT, i & 0xff);
         SHARED_ARRAY_FIELD_DP32(bus->regs, reg_cmd, RX_BUFF_EN, 0);
     } else if (SHARED_ARRAY_FIELD_EX32(bus->regs, reg_cmd, RX_DMA_EN)) {
-        uint8_t data;
         /* In new mode, clear how many bytes we RXed */
         if (aspeed_i2c_is_new_mode(bus->controller)) {
             ARRAY_FIELD_DP32(bus->regs, I2CM_DMA_LEN_STS, RX_LEN, 0);
-- 
2.41.0

Re: [PATCH 1/4] aspeed/i2c: Clean up local variable shadowing

[Philippe Mathieu-Daudé]

On 22/9/23 17:59, Cédric Le Goater wrote:
> Remove superfluous local 'data' variable and use the one define at the
> top of the routine. This fixes :
> 
>    ../hw/i2c/aspeed_i2c.c: In function ‘aspeed_i2c_bus_recv’:
>    ../hw/i2c/aspeed_i2c.c:315:17: warning: declaration of ‘data’ shadows a previous local [-Wshadow=compatible-local]
>      315 |         uint8_t data;
>          |                 ^~~~
>    ../hw/i2c/aspeed_i2c.c:288:13: note: shadowed declaration is here
>      288 |     uint8_t data;
>          |             ^~~~
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>   hw/i2c/aspeed_i2c.c | 1 -
>   1 file changed, 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 1/4] aspeed/i2c: Clean up local variable shadowing

[Joel Stanley]

On Fri, 22 Sept 2023 at 15:59, Cédric Le Goater <clg@kaod.org> wrote:
>
> Remove superfluous local 'data' variable and use the one define at the
> top of the routine. This fixes :
>
>   ../hw/i2c/aspeed_i2c.c: In function ‘aspeed_i2c_bus_recv’:
>   ../hw/i2c/aspeed_i2c.c:315:17: warning: declaration of ‘data’ shadows a previous local [-Wshadow=compatible-local]
>     315 |         uint8_t data;
>         |                 ^~~~
>   ../hw/i2c/aspeed_i2c.c:288:13: note: shadowed declaration is here
>     288 |     uint8_t data;
>         |             ^~~~
>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

Reviewed-by: Joel Stanley <joel@jms.id.au>

messy.

> ---
>  hw/i2c/aspeed_i2c.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
> index 7275d40749a9..1037c22b2f79 100644
> --- a/hw/i2c/aspeed_i2c.c
> +++ b/hw/i2c/aspeed_i2c.c
> @@ -312,7 +312,6 @@ static void aspeed_i2c_bus_recv(AspeedI2CBus *bus)
>          SHARED_ARRAY_FIELD_DP32(bus->regs, reg_pool_ctrl, RX_COUNT, i & 0xff);
>          SHARED_ARRAY_FIELD_DP32(bus->regs, reg_cmd, RX_BUFF_EN, 0);
>      } else if (SHARED_ARRAY_FIELD_EX32(bus->regs, reg_cmd, RX_DMA_EN)) {
> -        uint8_t data;
>          /* In new mode, clear how many bytes we RXed */
>          if (aspeed_i2c_is_new_mode(bus->controller)) {
>              ARRAY_FIELD_DP32(bus->regs, I2CM_DMA_LEN_STS, RX_LEN, 0);
> --
> 2.41.0
>

[PATCH 2/4] aspeed: Clean up local variable shadowing

[Cédric Le Goater]

Remove superfluous local 'irq' variables and use the one define at the
top of the routine. This fixes warnings in aspeed_soc_ast2600_realize()
such as :

  ../hw/arm/aspeed_ast2600.c: In function ‘aspeed_soc_ast2600_realize’:
  ../hw/arm/aspeed_ast2600.c:420:18: warning: declaration of ‘irq’ shadows a previous local [-Wshadow=compatible-local]
    420 |         qemu_irq irq = aspeed_soc_get_irq(s, ASPEED_DEV_TIMER1 + i);
        |                  ^~~
  ../hw/arm/aspeed_ast2600.c:312:14: note: shadowed declaration is here
    312 |     qemu_irq irq;
        |              ^~~

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/arm/aspeed_ast2600.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
index a8b3a8065a11..e122e1c32d42 100644
--- a/hw/arm/aspeed_ast2600.c
+++ b/hw/arm/aspeed_ast2600.c
@@ -388,7 +388,7 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp)
     aspeed_mmio_map(s, SYS_BUS_DEVICE(&s->timerctrl), 0,
                     sc->memmap[ASPEED_DEV_TIMER1]);
     for (i = 0; i < ASPEED_TIMER_NR_TIMERS; i++) {
-        qemu_irq irq = aspeed_soc_get_irq(s, ASPEED_DEV_TIMER1 + i);
+        irq = aspeed_soc_get_irq(s, ASPEED_DEV_TIMER1 + i);
         sysbus_connect_irq(SYS_BUS_DEVICE(&s->timerctrl), i, irq);
     }
 
@@ -413,8 +413,8 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp)
     }
     aspeed_mmio_map(s, SYS_BUS_DEVICE(&s->i2c), 0, sc->memmap[ASPEED_DEV_I2C]);
     for (i = 0; i < ASPEED_I2C_GET_CLASS(&s->i2c)->num_busses; i++) {
-        qemu_irq irq = qdev_get_gpio_in(DEVICE(&s->a7mpcore),
-                                        sc->irqmap[ASPEED_DEV_I2C] + i);
+        irq = qdev_get_gpio_in(DEVICE(&s->a7mpcore),
+                               sc->irqmap[ASPEED_DEV_I2C] + i);
         /* The AST2600 I2C controller has one IRQ per bus. */
         sysbus_connect_irq(SYS_BUS_DEVICE(&s->i2c.busses[i]), 0, irq);
     }
@@ -611,8 +611,8 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp)
     }
     aspeed_mmio_map(s, SYS_BUS_DEVICE(&s->i3c), 0, sc->memmap[ASPEED_DEV_I3C]);
     for (i = 0; i < ASPEED_I3C_NR_DEVICES; i++) {
-        qemu_irq irq = qdev_get_gpio_in(DEVICE(&s->a7mpcore),
-                                        sc->irqmap[ASPEED_DEV_I3C] + i);
+        irq = qdev_get_gpio_in(DEVICE(&s->a7mpcore),
+                               sc->irqmap[ASPEED_DEV_I3C] + i);
         /* The AST2600 I3C controller has one IRQ per bus. */
         sysbus_connect_irq(SYS_BUS_DEVICE(&s->i3c.devices[i]), 0, irq);
     }
-- 
2.41.0

Re: [PATCH 2/4] aspeed: Clean up local variable shadowing

[Philippe Mathieu-Daudé]

On 22/9/23 17:59, Cédric Le Goater wrote:
> Remove superfluous local 'irq' variables and use the one define at the
> top of the routine. This fixes warnings in aspeed_soc_ast2600_realize()
> such as :
> 
>    ../hw/arm/aspeed_ast2600.c: In function ‘aspeed_soc_ast2600_realize’:
>    ../hw/arm/aspeed_ast2600.c:420:18: warning: declaration of ‘irq’ shadows a previous local [-Wshadow=compatible-local]
>      420 |         qemu_irq irq = aspeed_soc_get_irq(s, ASPEED_DEV_TIMER1 + i);
>          |                  ^~~
>    ../hw/arm/aspeed_ast2600.c:312:14: note: shadowed declaration is here
>      312 |     qemu_irq irq;
>          |              ^~~
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>   hw/arm/aspeed_ast2600.c | 10 +++++-----
>   1 file changed, 5 insertions(+), 5 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 2/4] aspeed: Clean up local variable shadowing

[Markus Armbruster]

Cédric Le Goater <clg@kaod.org> writes:

> Remove superfluous local 'irq' variables and use the one define at the
> top of the routine. This fixes warnings in aspeed_soc_ast2600_realize()
> such as :
>
>   ../hw/arm/aspeed_ast2600.c: In function ‘aspeed_soc_ast2600_realize’:
>   ../hw/arm/aspeed_ast2600.c:420:18: warning: declaration of ‘irq’ shadows a previous local [-Wshadow=compatible-local]
>     420 |         qemu_irq irq = aspeed_soc_get_irq(s, ASPEED_DEV_TIMER1 + i);
>         |                  ^~~
>   ../hw/arm/aspeed_ast2600.c:312:14: note: shadowed declaration is here
>     312 |     qemu_irq irq;
>         |              ^~~
>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>  hw/arm/aspeed_ast2600.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
> index a8b3a8065a11..e122e1c32d42 100644
> --- a/hw/arm/aspeed_ast2600.c
> +++ b/hw/arm/aspeed_ast2600.c
> @@ -388,7 +388,7 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp)
>      aspeed_mmio_map(s, SYS_BUS_DEVICE(&s->timerctrl), 0,
>                      sc->memmap[ASPEED_DEV_TIMER1]);
>      for (i = 0; i < ASPEED_TIMER_NR_TIMERS; i++) {
> -        qemu_irq irq = aspeed_soc_get_irq(s, ASPEED_DEV_TIMER1 + i);
> +        irq = aspeed_soc_get_irq(s, ASPEED_DEV_TIMER1 + i);
>          sysbus_connect_irq(SYS_BUS_DEVICE(&s->timerctrl), i, irq);
>      }
>  
> @@ -413,8 +413,8 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp)
>      }
>      aspeed_mmio_map(s, SYS_BUS_DEVICE(&s->i2c), 0, sc->memmap[ASPEED_DEV_I2C]);
>      for (i = 0; i < ASPEED_I2C_GET_CLASS(&s->i2c)->num_busses; i++) {
> -        qemu_irq irq = qdev_get_gpio_in(DEVICE(&s->a7mpcore),
> -                                        sc->irqmap[ASPEED_DEV_I2C] + i);
> +        irq = qdev_get_gpio_in(DEVICE(&s->a7mpcore),
> +                               sc->irqmap[ASPEED_DEV_I2C] + i);
>          /* The AST2600 I2C controller has one IRQ per bus. */
>          sysbus_connect_irq(SYS_BUS_DEVICE(&s->i2c.busses[i]), 0, irq);
>      }
> @@ -611,8 +611,8 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp)
>      }
>      aspeed_mmio_map(s, SYS_BUS_DEVICE(&s->i3c), 0, sc->memmap[ASPEED_DEV_I3C]);
>      for (i = 0; i < ASPEED_I3C_NR_DEVICES; i++) {
> -        qemu_irq irq = qdev_get_gpio_in(DEVICE(&s->a7mpcore),
> -                                        sc->irqmap[ASPEED_DEV_I3C] + i);
> +        irq = qdev_get_gpio_in(DEVICE(&s->a7mpcore),
> +                               sc->irqmap[ASPEED_DEV_I3C] + i);
>          /* The AST2600 I3C controller has one IRQ per bus. */
>          sysbus_connect_irq(SYS_BUS_DEVICE(&s->i3c.devices[i]), 0, irq);
>      }

Clashes with Philippe's

    [PATCH v2 10/22] hw/arm/aspeed: Clean up local variable shadowing
    Message-ID: <20230904161235.84651-11-philmd@linaro.org>

The difference is a matter of taste: one @irq in function scope vs. four
in nested scopes.  I'd prefer the former, i.e. this patch, but
maintainers' preference matter more than mine.  If you want me to merge
together with other shadowing patches, tell me your preference, if any.

Re: [PATCH 2/4] aspeed: Clean up local variable shadowing

[Cédric Le Goater]

On 9/23/23 08:34, Markus Armbruster wrote:
> Cédric Le Goater <clg@kaod.org> writes:
> 
>> Remove superfluous local 'irq' variables and use the one define at the
>> top of the routine. This fixes warnings in aspeed_soc_ast2600_realize()
>> such as :
>>
>>    ../hw/arm/aspeed_ast2600.c: In function ‘aspeed_soc_ast2600_realize’:
>>    ../hw/arm/aspeed_ast2600.c:420:18: warning: declaration of ‘irq’ shadows a previous local [-Wshadow=compatible-local]
>>      420 |         qemu_irq irq = aspeed_soc_get_irq(s, ASPEED_DEV_TIMER1 + i);
>>          |                  ^~~
>>    ../hw/arm/aspeed_ast2600.c:312:14: note: shadowed declaration is here
>>      312 |     qemu_irq irq;
>>          |              ^~~
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>   hw/arm/aspeed_ast2600.c | 10 +++++-----
>>   1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
>> index a8b3a8065a11..e122e1c32d42 100644
>> --- a/hw/arm/aspeed_ast2600.c
>> +++ b/hw/arm/aspeed_ast2600.c
>> @@ -388,7 +388,7 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp)
>>       aspeed_mmio_map(s, SYS_BUS_DEVICE(&s->timerctrl), 0,
>>                       sc->memmap[ASPEED_DEV_TIMER1]);
>>       for (i = 0; i < ASPEED_TIMER_NR_TIMERS; i++) {
>> -        qemu_irq irq = aspeed_soc_get_irq(s, ASPEED_DEV_TIMER1 + i);
>> +        irq = aspeed_soc_get_irq(s, ASPEED_DEV_TIMER1 + i);
>>           sysbus_connect_irq(SYS_BUS_DEVICE(&s->timerctrl), i, irq);
>>       }
>>   
>> @@ -413,8 +413,8 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp)
>>       }
>>       aspeed_mmio_map(s, SYS_BUS_DEVICE(&s->i2c), 0, sc->memmap[ASPEED_DEV_I2C]);
>>       for (i = 0; i < ASPEED_I2C_GET_CLASS(&s->i2c)->num_busses; i++) {
>> -        qemu_irq irq = qdev_get_gpio_in(DEVICE(&s->a7mpcore),
>> -                                        sc->irqmap[ASPEED_DEV_I2C] + i);
>> +        irq = qdev_get_gpio_in(DEVICE(&s->a7mpcore),
>> +                               sc->irqmap[ASPEED_DEV_I2C] + i);
>>           /* The AST2600 I2C controller has one IRQ per bus. */
>>           sysbus_connect_irq(SYS_BUS_DEVICE(&s->i2c.busses[i]), 0, irq);
>>       }
>> @@ -611,8 +611,8 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp)
>>       }
>>       aspeed_mmio_map(s, SYS_BUS_DEVICE(&s->i3c), 0, sc->memmap[ASPEED_DEV_I3C]);
>>       for (i = 0; i < ASPEED_I3C_NR_DEVICES; i++) {
>> -        qemu_irq irq = qdev_get_gpio_in(DEVICE(&s->a7mpcore),
>> -                                        sc->irqmap[ASPEED_DEV_I3C] + i);
>> +        irq = qdev_get_gpio_in(DEVICE(&s->a7mpcore),
>> +                               sc->irqmap[ASPEED_DEV_I3C] + i);
>>           /* The AST2600 I3C controller has one IRQ per bus. */
>>           sysbus_connect_irq(SYS_BUS_DEVICE(&s->i3c.devices[i]), 0, irq);
>>       }
> 
> Clashes with Philippe's
> 
>      [PATCH v2 10/22] hw/arm/aspeed: Clean up local variable shadowing
>      Message-ID: <20230904161235.84651-11-philmd@linaro.org>
> 
> The difference is a matter of taste: one @irq in function scope vs. four
> in nested scopes.  I'd prefer the former, i.e. this patch, but
> maintainers' preference matter more than mine.  If you want me to merge
> together with other shadowing patches, tell me your preference, if any.

You are the one gathering patches. Please choose. I don't mind.

Thanks,

C.

[PATCH 3/4] aspeed/i3c: Rename variable shadowing a local

[Cédric Le Goater]

to fix warning :

  ../hw/i3c/aspeed_i3c.c: In function ‘aspeed_i3c_realize’:
  ../hw/i3c/aspeed_i3c.c:1959:17: warning: declaration of ‘dev’ shadows a parameter [-Wshadow=local]
   1959 |         Object *dev = OBJECT(&s->devices[i]);
        |                 ^~~
  ../hw/i3c/aspeed_i3c.c:1942:45: note: shadowed declaration is here
   1942 | static void aspeed_i3c_realize(DeviceState *dev, Error **errp)
        |                                ~~~~~~~~~~~~~^~~

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/misc/aspeed_i3c.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/misc/aspeed_i3c.c b/hw/misc/aspeed_i3c.c
index f54f5da522b3..d1ff61767167 100644
--- a/hw/misc/aspeed_i3c.c
+++ b/hw/misc/aspeed_i3c.c
@@ -296,13 +296,13 @@ static void aspeed_i3c_realize(DeviceState *dev, Error **errp)
     memory_region_add_subregion(&s->iomem_container, 0x0, &s->iomem);
 
     for (i = 0; i < ASPEED_I3C_NR_DEVICES; ++i) {
-        Object *dev = OBJECT(&s->devices[i]);
+        Object *i3c_dev = OBJECT(&s->devices[i]);
 
-        if (!object_property_set_uint(dev, "device-id", i, errp)) {
+        if (!object_property_set_uint(i3c_dev, "device-id", i, errp)) {
             return;
         }
 
-        if (!sysbus_realize(SYS_BUS_DEVICE(dev), errp)) {
+        if (!sysbus_realize(SYS_BUS_DEVICE(i3c_dev), errp)) {
             return;
         }
 
-- 
2.41.0

Re: [PATCH 3/4] aspeed/i3c: Rename variable shadowing a local

[Philippe Mathieu-Daudé]

On 22/9/23 17:59, Cédric Le Goater wrote:
> to fix warning :
> 
>    ../hw/i3c/aspeed_i3c.c: In function ‘aspeed_i3c_realize’:
>    ../hw/i3c/aspeed_i3c.c:1959:17: warning: declaration of ‘dev’ shadows a parameter [-Wshadow=local]
>     1959 |         Object *dev = OBJECT(&s->devices[i]);
>          |                 ^~~
>    ../hw/i3c/aspeed_i3c.c:1942:45: note: shadowed declaration is here
>     1942 | static void aspeed_i3c_realize(DeviceState *dev, Error **errp)
>          |                                ~~~~~~~~~~~~~^~~
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>   hw/misc/aspeed_i3c.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/misc/aspeed_i3c.c b/hw/misc/aspeed_i3c.c
> index f54f5da522b3..d1ff61767167 100644
> --- a/hw/misc/aspeed_i3c.c
> +++ b/hw/misc/aspeed_i3c.c
> @@ -296,13 +296,13 @@ static void aspeed_i3c_realize(DeviceState *dev, Error **errp)

Alternatively:

-- >8 --

-static void aspeed_i3c_realize(DeviceState *dev, Error **errp)
+static void aspeed_i3c_realize(DeviceState *ctrl, Error **errp)
  {
      int i;
-    AspeedI3CState *s = ASPEED_I3C(dev);
-    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
+    AspeedI3CState *s = ASPEED_I3C(ctrl);
+    SysBusDevice *sbd = SYS_BUS_DEVICE(ctrl);

---

>       memory_region_add_subregion(&s->iomem_container, 0x0, &s->iomem);
>   
>       for (i = 0; i < ASPEED_I3C_NR_DEVICES; ++i) {
> -        Object *dev = OBJECT(&s->devices[i]);
> +        Object *i3c_dev = OBJECT(&s->devices[i]);
>   
> -        if (!object_property_set_uint(dev, "device-id", i, errp)) {
> +        if (!object_property_set_uint(i3c_dev, "device-id", i, errp)) {
>               return;
>           }
>   
> -        if (!sysbus_realize(SYS_BUS_DEVICE(dev), errp)) {
> +        if (!sysbus_realize(SYS_BUS_DEVICE(i3c_dev), errp)) {
>               return;
>           }
>   

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 3/4] aspeed/i3c: Rename variable shadowing a local

[Andrew Jeffery]

On Sat, 23 Sep 2023, at 03:58, Philippe Mathieu-Daudé wrote:
> On 22/9/23 17:59, Cédric Le Goater wrote:
>> to fix warning :
>> 
>>    ../hw/i3c/aspeed_i3c.c: In function ‘aspeed_i3c_realize’:
>>    ../hw/i3c/aspeed_i3c.c:1959:17: warning: declaration of ‘dev’ shadows a parameter [-Wshadow=local]
>>     1959 |         Object *dev = OBJECT(&s->devices[i]);
>>          |                 ^~~
>>    ../hw/i3c/aspeed_i3c.c:1942:45: note: shadowed declaration is here
>>     1942 | static void aspeed_i3c_realize(DeviceState *dev, Error **errp)
>>          |                                ~~~~~~~~~~~~~^~~
>> 
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>   hw/misc/aspeed_i3c.c | 6 +++---
>>   1 file changed, 3 insertions(+), 3 deletions(-)
>> 
>> diff --git a/hw/misc/aspeed_i3c.c b/hw/misc/aspeed_i3c.c
>> index f54f5da522b3..d1ff61767167 100644
>> --- a/hw/misc/aspeed_i3c.c
>> +++ b/hw/misc/aspeed_i3c.c
>> @@ -296,13 +296,13 @@ static void aspeed_i3c_realize(DeviceState *dev, Error **errp)
>
> Alternatively:
>
> -- >8 --
>
> -static void aspeed_i3c_realize(DeviceState *dev, Error **errp)
> +static void aspeed_i3c_realize(DeviceState *ctrl, Error **errp)
>   {
>       int i;
> -    AspeedI3CState *s = ASPEED_I3C(dev);
> -    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> +    AspeedI3CState *s = ASPEED_I3C(ctrl);
> +    SysBusDevice *sbd = SYS_BUS_DEVICE(ctrl);

Hmm, I feel like `dev` isn't an unreasonable or unusual name for the formal parameter? Switching to `ctrl` isn't my immediate preference.

>
> ---
>
>>       memory_region_add_subregion(&s->iomem_container, 0x0, &s->iomem);
>>   
>>       for (i = 0; i < ASPEED_I3C_NR_DEVICES; ++i) {
>> -        Object *dev = OBJECT(&s->devices[i]);
>> +        Object *i3c_dev = OBJECT(&s->devices[i]);

Maybe `s/i3c_dev/subdev`? I dunno. That bikeshed isn't gonna get painted on its own.

Reviewed-by: Andrew Jeffery <andrew@aj.id.au>

>>   
>> -        if (!object_property_set_uint(dev, "device-id", i, errp)) {
>> +        if (!object_property_set_uint(i3c_dev, "device-id", i, errp)) {
>>               return;
>>           }
>>   
>> -        if (!sysbus_realize(SYS_BUS_DEVICE(dev), errp)) {
>> +        if (!sysbus_realize(SYS_BUS_DEVICE(i3c_dev), errp)) {
>>               return;
>>           }
>>   
>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH 4/4] aspeed/timer: Clean up local variable shadowing

[Cédric Le Goater]

commit 8137355e850f ("aspeed/timer: Fix behaviour running Linux")
introduced a MAX() expression to calculate the next timer deadline :

    return calculate_time(t, MAX(MAX(t->match[0], t->match[1]), 0));

The second MAX() is not necessary since the compared values are an
unsigned and 0. Simply remove it and fix warning :

  ../hw/timer/aspeed_timer.c: In function ‘calculate_next’:
  ../include/qemu/osdep.h:396:31: warning: declaration of ‘_a’ shadows a previous local [-Wshadow=compatible-local]
    396 |         typeof(1 ? (a) : (b)) _a = (a), _b = (b);       \
        |                               ^~
  ../hw/timer/aspeed_timer.c:170:12: note: in expansion of macro ‘MAX’
    170 |     next = MAX(MAX(calculate_match(t, 0), calculate_match(t, 1)), 0);
        |            ^~~
  ../hw/timer/aspeed_timer.c:170:16: note: in expansion of macro ‘MAX’
    170 |     next = MAX(MAX(calculate_match(t, 0), calculate_match(t, 1)), 0);
        |                ^~~
  /home/legoater/work/qemu/qemu-aspeed.git/include/qemu/osdep.h:396:31: note: shadowed declaration is here
    396 |         typeof(1 ? (a) : (b)) _a = (a), _b = (b);       \
        |                               ^~
  ../hw/timer/aspeed_timer.c:170:12: note: in expansion of macro ‘MAX’
    170 |     next = MAX(MAX(calculate_match(t, 0), calculate_match(t, 1)), 0);
        |            ^~~

Cc: Joel Stanley <joel@jms.id.au>
Cc: Andrew Jeffery <andrew@aj.id.au>
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/timer/aspeed_timer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/timer/aspeed_timer.c b/hw/timer/aspeed_timer.c
index 9c20b3d6ad8a..72161f07bbee 100644
--- a/hw/timer/aspeed_timer.c
+++ b/hw/timer/aspeed_timer.c
@@ -167,7 +167,7 @@ static uint64_t calculate_next(struct AspeedTimer *t)
         qemu_set_irq(t->irq, t->level);
     }
 
-    next = MAX(MAX(calculate_match(t, 0), calculate_match(t, 1)), 0);
+    next = MAX(calculate_match(t, 0), calculate_match(t, 1));
     t->start = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
 
     return calculate_time(t, next);
-- 
2.41.0

Re: [PATCH 4/4] aspeed/timer: Clean up local variable shadowing

[Philippe Mathieu-Daudé]

On 22/9/23 17:59, Cédric Le Goater wrote:
> commit 8137355e850f ("aspeed/timer: Fix behaviour running Linux")
> introduced a MAX() expression to calculate the next timer deadline :
> 
>      return calculate_time(t, MAX(MAX(t->match[0], t->match[1]), 0));
> 
> The second MAX() is not necessary since the compared values are an
> unsigned and 0. Simply remove it and fix warning :
> 
>    ../hw/timer/aspeed_timer.c: In function ‘calculate_next’:
>    ../include/qemu/osdep.h:396:31: warning: declaration of ‘_a’ shadows a previous local [-Wshadow=compatible-local]
>      396 |         typeof(1 ? (a) : (b)) _a = (a), _b = (b);       \
>          |                               ^~
>    ../hw/timer/aspeed_timer.c:170:12: note: in expansion of macro ‘MAX’
>      170 |     next = MAX(MAX(calculate_match(t, 0), calculate_match(t, 1)), 0);
>          |            ^~~
>    ../hw/timer/aspeed_timer.c:170:16: note: in expansion of macro ‘MAX’
>      170 |     next = MAX(MAX(calculate_match(t, 0), calculate_match(t, 1)), 0);
>          |                ^~~
>    /home/legoater/work/qemu/qemu-aspeed.git/include/qemu/osdep.h:396:31: note: shadowed declaration is here
>      396 |         typeof(1 ? (a) : (b)) _a = (a), _b = (b);       \
>          |                               ^~
>    ../hw/timer/aspeed_timer.c:170:12: note: in expansion of macro ‘MAX’
>      170 |     next = MAX(MAX(calculate_match(t, 0), calculate_match(t, 1)), 0);
>          |            ^~~
> 
> Cc: Joel Stanley <joel@jms.id.au>
> Cc: Andrew Jeffery <andrew@aj.id.au>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>   hw/timer/aspeed_timer.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 0/4] aspeed: Clean up local variable shadowing

[Philippe Mathieu-Daudé]

On 22/9/23 17:59, Cédric Le Goater wrote:
> Hello,
> 
> Here are cleanups for local variable shadowing warnings in aspeed models.
> 
> Joel, Andrew,
> 
> Could you please double check patch 4 ?

Could Markus' MAKE_IDENTFIER() help there?

> 
> Thanks,
> 
> C.
> 
> Cédric Le Goater (4):
>    aspeed/i2c: Clean up local variable shadowing
>    aspeed: Clean up local variable shadowing
>    aspeed/i3c: Rename variable shadowing a local
>    aspeed/timer: Clean up local variable shadowing
> 
>   hw/arm/aspeed_ast2600.c | 10 +++++-----
>   hw/i2c/aspeed_i2c.c     |  1 -
>   hw/misc/aspeed_i3c.c    |  6 +++---
>   hw/timer/aspeed_timer.c |  2 +-
>   4 files changed, 9 insertions(+), 10 deletions(-)
> 

Re: [PATCH 0/4] aspeed: Clean up local variable shadowing

[Cédric Le Goater]

On 9/22/23 20:20, Philippe Mathieu-Daudé wrote:
> On 22/9/23 17:59, Cédric Le Goater wrote:
>> Hello,
>>
>> Here are cleanups for local variable shadowing warnings in aspeed models.
>>
>> Joel, Andrew,
>>
>> Could you please double check patch 4 ?
> 
> Could Markus' MAKE_IDENTFIER() help there?

ah ! you typed too fast and I also read too fast, as :

   MARKUS_IDENTIFIER()

and I liked it :) but what is MAKE_IDENTIFIER  ? really, please explain.

Thanks,

C.

Re: [PATCH 0/4] aspeed: Clean up local variable shadowing

[Markus Armbruster]

Cédric Le Goater <clg@kaod.org> writes:

> On 9/22/23 20:20, Philippe Mathieu-Daudé wrote:
>> On 22/9/23 17:59, Cédric Le Goater wrote:
>>> Hello,
>>>
>>> Here are cleanups for local variable shadowing warnings in aspeed models.
>>>
>>> Joel, Andrew,
>>>
>>> Could you please double check patch 4 ?
>> Could Markus' MAKE_IDENTFIER() help there?
>
> ah ! you typed too fast and I also read too fast, as :
>
>   MARKUS_IDENTIFIER()
>
> and I liked it :)

LOL

>                   but what is MAKE_IDENTIFIER  ? really, please explain.

Philippe is referring to

    [PATCH v3 7/7] qobject atomics osdep: Make a few macros more hygienic
    Message-ID: <20230921121312.1301864-8-armbru@redhat.com>

which tweaks MAX() to permit nesting without shadowing.  Your PATCH 4
may not be needed if you base on it.

MAKE_IDENTIFIER() is a helper macro introduced in that patch.

You can fetch the patch from https://repo.or.cz/qemu/armbru.git branch
shadow-next, along with collected other shadowing patches.

Questions?

Re: [PATCH 0/4] aspeed: Clean up local variable shadowing

[Cédric Le Goater]

On 9/23/23 09:13, Markus Armbruster wrote:
> Cédric Le Goater <clg@kaod.org> writes:
> 
>> On 9/22/23 20:20, Philippe Mathieu-Daudé wrote:
>>> On 22/9/23 17:59, Cédric Le Goater wrote:
>>>> Hello,
>>>>
>>>> Here are cleanups for local variable shadowing warnings in aspeed models.
>>>>
>>>> Joel, Andrew,
>>>>
>>>> Could you please double check patch 4 ?
>>> Could Markus' MAKE_IDENTFIER() help there?
>>
>> ah ! you typed too fast and I also read too fast, as :
>>
>>    MARKUS_IDENTIFIER()
>>
>> and I liked it :)
> 
> LOL
> 
>>                    but what is MAKE_IDENTIFIER  ? really, please explain.
> 
> Philippe is referring to
> 
>      [PATCH v3 7/7] qobject atomics osdep: Make a few macros more hygienic
>      Message-ID: <20230921121312.1301864-8-armbru@redhat.com>
> 
> which tweaks MAX() to permit nesting without shadowing.  Your PATCH 4
> may not be needed if you base on it.

We don't need the nested MAX(). PATCH 4 is a cleanup which happens
to remove the shadowing. Let's keep it the way it is.

> MAKE_IDENTIFIER() is a helper macro introduced in that patch.
> 
> You can fetch the patch from https://repo.or.cz/qemu/armbru.git branch
> shadow-next, along with collected other shadowing patches.

OK.

Would you prefer maintainers to include the shadowing changes in a
potential PR they would send or would you rather take care of it in
a PR of your own ?

Thanks,

C.

Re: [PATCH 0/4] aspeed: Clean up local variable shadowing

[Markus Armbruster]

Cédric Le Goater <clg@kaod.org> writes:

> On 9/23/23 09:13, Markus Armbruster wrote:
>> Cédric Le Goater <clg@kaod.org> writes:
>> 
>>> On 9/22/23 20:20, Philippe Mathieu-Daudé wrote:
>>>> On 22/9/23 17:59, Cédric Le Goater wrote:
>>>>> Hello,
>>>>>
>>>>> Here are cleanups for local variable shadowing warnings in aspeed models.
>>>>>
>>>>> Joel, Andrew,
>>>>>
>>>>> Could you please double check patch 4 ?
>>>> Could Markus' MAKE_IDENTFIER() help there?
>>>
>>> ah ! you typed too fast and I also read too fast, as :
>>>
>>>    MARKUS_IDENTIFIER()
>>>
>>> and I liked it :)
>>
>> LOL
>> 
>>>                    but what is MAKE_IDENTIFIER  ? really, please explain.
>>
>> Philippe is referring to
>>
>>      [PATCH v3 7/7] qobject atomics osdep: Make a few macros more hygienic
>>      Message-ID: <20230921121312.1301864-8-armbru@redhat.com>
>>
>> which tweaks MAX() to permit nesting without shadowing.  Your PATCH 4
>> may not be needed if you base on it.
>
> We don't need the nested MAX(). PATCH 4 is a cleanup which happens
> to remove the shadowing. Let's keep it the way it is.
>
>> MAKE_IDENTIFIER() is a helper macro introduced in that patch.
>> You can fetch the patch from https://repo.or.cz/qemu/armbru.git branch
>> shadow-next, along with collected other shadowing patches.
>
> OK.
>
> Would you prefer maintainers to include the shadowing changes in a
> potential PR they would send or would you rather take care of it in
> a PR of your own ?

I'm happy to collect patches and do pull requests.  I don't mind
maintainers merging patches for their subsystems; interference should be
minimal.

Thanks!

Re: [PATCH 0/4] aspeed: Clean up local variable shadowing

[Markus Armbruster]

Cédric Le Goater <clg@kaod.org> writes:

> Hello,
>
> Here are cleanups for local variable shadowing warnings in aspeed models.
>
> Joel, Andrew,
>
> Could you please double check patch 4 ? 
>
> Thanks,
>
> C. 

Queued, thanks!