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

Cédric Le Goater posted 8 patches 5 months, 1 week ago
Maintainers: "Cédric Le Goater" <clg@kaod.org>, Nicholas Piggin <npiggin@gmail.com>, "Frédéric Barrat" <fbarrat@linux.ibm.com>, Daniel Henrique Barboza <danielhb413@gmail.com>, David Gibson <david@gibson.dropbear.id.au>, Harsh Prateek Bora <harshpb@linux.ibm.com>
[PATCH 2/8] pnv/psi: Clean up local variable shadowing
Posted by Cédric Le Goater 5 months, 1 week ago
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 =
+                    val & ~(PSIHB9_ESB_CI_VALID | PSIHB10_ESB_CI_64K);
+                memory_region_add_subregion(sysmem, esb_addr,
                                             &psi9->source.esb_mmio);
             }
         }
-- 
2.41.0


Re: [PATCH 2/8] pnv/psi: Clean up local variable shadowing
Posted by Harsh Prateek Bora 5 months, 1 week ago

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. Anyways,

Reviewed-by: Harsh Prateek Bora <harshpb@linux.ibm.com>

> +                    val & ~(PSIHB9_ESB_CI_VALID | PSIHB10_ESB_CI_64K);
> +                memory_region_add_subregion(sysmem, esb_addr,
>                                               &psi9->source.esb_mmio);
>               }
>           }

Re: [PATCH 2/8] pnv/psi: Clean up local variable shadowing
Posted by Cédric Le Goater 5 months, 1 week ago
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.

> 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);
>>               }
>>           }


Re: [PATCH 2/8] pnv/psi: Clean up local variable shadowing
Posted by Markus Armbruster 4 months, 4 weeks ago
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.  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);
>>>               }
>>>           }
Re: [PATCH 2/8] pnv/psi: Clean up local variable shadowing
Posted by Cédric Le Goater 4 months, 4 weeks ago
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);
>>>>                }
>>>>            }