[PATCH 6/8] spapr/drc: Clean up local variable shadowing in rtas_ibm_configure_connector()

Cédric Le Goater posted 8 patches 8 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 6/8] spapr/drc: Clean up local variable shadowing in rtas_ibm_configure_connector()
Posted by Cédric Le Goater 8 months, 1 week ago
Remove extra 'drc_index' variable to avoid this warning :

  ../hw/ppc/spapr_drc.c: In function ‘rtas_ibm_configure_connector’:
  ../hw/ppc/spapr_drc.c:1240:26: warning: declaration of ‘drc_index’ shadows a previous local [-Wshadow=compatible-local]
   1240 |                 uint32_t drc_index = spapr_drc_index(drc);
        |                          ^~~~~~~~~
  ../hw/ppc/spapr_drc.c:1155:14: note: shadowed declaration is here
   1155 |     uint32_t drc_index;
        |              ^~~~~~~~~

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/ppc/spapr_drc.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index b5c400a94d1c..843e318312d3 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -1237,8 +1237,6 @@ static void rtas_ibm_configure_connector(PowerPCCPU *cpu,
         case FDT_END_NODE:
             drc->ccs_depth--;
             if (drc->ccs_depth == 0) {
-                uint32_t drc_index = spapr_drc_index(drc);
-
                 /* done sending the device tree, move to configured state */
                 trace_spapr_drc_set_configured(drc_index);
                 drc->state = drck->ready_state;
-- 
2.41.0


Re: [PATCH 6/8] spapr/drc: Clean up local variable shadowing in rtas_ibm_configure_connector()
Posted by Harsh Prateek Bora 8 months, 1 week ago

On 9/18/23 20:28, Cédric Le Goater wrote:
> Remove extra 'drc_index' variable to avoid this warning :
> 
>    ../hw/ppc/spapr_drc.c: In function ‘rtas_ibm_configure_connector’:
>    ../hw/ppc/spapr_drc.c:1240:26: warning: declaration of ‘drc_index’ shadows a previous local [-Wshadow=compatible-local]
>     1240 |                 uint32_t drc_index = spapr_drc_index(drc);
>          |                          ^~~~~~~~~
>    ../hw/ppc/spapr_drc.c:1155:14: note: shadowed declaration is here
>     1155 |     uint32_t drc_index;
>          |              ^~~~~~~~~
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>   hw/ppc/spapr_drc.c | 2 --
>   1 file changed, 2 deletions(-)
> 
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index b5c400a94d1c..843e318312d3 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -1237,8 +1237,6 @@ static void rtas_ibm_configure_connector(PowerPCCPU *cpu,
>           case FDT_END_NODE:
>               drc->ccs_depth--;
>               if (drc->ccs_depth == 0) {
> -                uint32_t drc_index = spapr_drc_index(drc);
> -
I guess you only wanted to remove re-declaration part. Assigning the 
value returned by this function doesnt seem to happen before.

>                   /* done sending the device tree, move to configured state */
>                   trace_spapr_drc_set_configured(drc_index);
>                   drc->state = drck->ready_state;

Re: [PATCH 6/8] spapr/drc: Clean up local variable shadowing in rtas_ibm_configure_connector()
Posted by Cédric Le Goater 8 months, 1 week ago
On 9/19/23 10:29, Harsh Prateek Bora wrote:
> 
> 
> On 9/18/23 20:28, Cédric Le Goater wrote:
>> Remove extra 'drc_index' variable to avoid this warning :
>>
>>    ../hw/ppc/spapr_drc.c: In function ‘rtas_ibm_configure_connector’:
>>    ../hw/ppc/spapr_drc.c:1240:26: warning: declaration of ‘drc_index’ shadows a previous local [-Wshadow=compatible-local]
>>     1240 |                 uint32_t drc_index = spapr_drc_index(drc);
>>          |                          ^~~~~~~~~
>>    ../hw/ppc/spapr_drc.c:1155:14: note: shadowed declaration is here
>>     1155 |     uint32_t drc_index;
>>          |              ^~~~~~~~~
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>   hw/ppc/spapr_drc.c | 2 --
>>   1 file changed, 2 deletions(-)
>>
>> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
>> index b5c400a94d1c..843e318312d3 100644
>> --- a/hw/ppc/spapr_drc.c
>> +++ b/hw/ppc/spapr_drc.c
>> @@ -1237,8 +1237,6 @@ static void rtas_ibm_configure_connector(PowerPCCPU *cpu,
>>           case FDT_END_NODE:
>>               drc->ccs_depth--;
>>               if (drc->ccs_depth == 0) {
>> -                uint32_t drc_index = spapr_drc_index(drc);
>> -
> I guess you only wanted to remove re-declaration part. Assigning the value returned by this function doesnt seem to happen before.

drc_index is assigned at the top of this large routine with :

     drc_index = rtas_ld(wa_addr, 0);
     drc = spapr_drc_by_index(drc_index);


So, the extra local variable 'drc_index' is simply redundant because
there are no reason for it to change. The drc object is the same AFAICT.
Correct ? I should have explained that better in the commit log.

Thanks,

C.


> 
>>                   /* done sending the device tree, move to configured state */
>>                   trace_spapr_drc_set_configured(drc_index);
>>                   drc->state = drck->ready_state;


Re: [PATCH 6/8] spapr/drc: Clean up local variable shadowing in rtas_ibm_configure_connector()
Posted by Harsh Prateek Bora 8 months, 1 week ago
On Tue, 19 Sept, 2023, 5:33 pm Cédric Le Goater, <clg@kaod.org> wrote:

> On 9/19/23 10:29, Harsh Prateek Bora wrote:
> >
> >
> > On 9/18/23 20:28, Cédric Le Goater wrote:
> >> Remove extra 'drc_index' variable to avoid this warning :
> >>
> >>    ../hw/ppc/spapr_drc.c: In function ‘rtas_ibm_configure_connector’:
> >>    ../hw/ppc/spapr_drc.c:1240:26: warning: declaration of ‘drc_index’
> shadows a previous local [-Wshadow=compatible-local]
> >>     1240 |                 uint32_t drc_index = spapr_drc_index(drc);
> >>          |                          ^~~~~~~~~
> >>    ../hw/ppc/spapr_drc.c:1155:14: note: shadowed declaration is here
> >>     1155 |     uint32_t drc_index;
> >>          |              ^~~~~~~~~
> >>
> >> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> >> ---
> >>   hw/ppc/spapr_drc.c | 2 --
> >>   1 file changed, 2 deletions(-)
> >>
> >> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> >> index b5c400a94d1c..843e318312d3 100644
> >> --- a/hw/ppc/spapr_drc.c
> >> +++ b/hw/ppc/spapr_drc.c
> >> @@ -1237,8 +1237,6 @@ static void
> rtas_ibm_configure_connector(PowerPCCPU *cpu,
> >>           case FDT_END_NODE:
> >>               drc->ccs_depth--;
> >>               if (drc->ccs_depth == 0) {
> >> -                uint32_t drc_index = spapr_drc_index(drc);
> >> -
> > I guess you only wanted to remove re-declaration part. Assigning the
> value returned by this function doesnt seem to happen before.
>
> drc_index is assigned at the top of this large routine with :
>
>      drc_index = rtas_ld(wa_addr, 0);
>      drc = spapr_drc_by_index(drc_index);
>
>
> So, the extra local variable 'drc_index' is simply redundant because
> there are no reason for it to change. The drc object is the same AFAICT.
> Correct ? I should have explained that better in the commit log.
>

Okay, since both routines were implemented differently, I wasn't sure about
the impact of reassignment. Better commit log is always welcome.

Regards
Harsh

Thanks,
>
> C.
>
>
> >
> >>                   /* done sending the device tree, move to configured
> state */
> >>                   trace_spapr_drc_set_configured(drc_index);
> >>                   drc->state = drck->ready_state;
>
>
>
Re: [PATCH 6/8] spapr/drc: Clean up local variable shadowing in rtas_ibm_configure_connector()
Posted by Markus Armbruster 7 months, 4 weeks ago
Harsh Prateek Bora <harsh.prateek.bora@gmail.com> writes:

> On Tue, 19 Sept, 2023, 5:33 pm Cédric Le Goater, <clg@kaod.org> wrote:
>
>> On 9/19/23 10:29, Harsh Prateek Bora wrote:
>> >
>> >
>> > On 9/18/23 20:28, Cédric Le Goater wrote:
>> >> Remove extra 'drc_index' variable to avoid this warning :
>> >>
>> >>    ../hw/ppc/spapr_drc.c: In function ‘rtas_ibm_configure_connector’:
>> >>    ../hw/ppc/spapr_drc.c:1240:26: warning: declaration of ‘drc_index’
>> shadows a previous local [-Wshadow=compatible-local]
>> >>     1240 |                 uint32_t drc_index = spapr_drc_index(drc);
>> >>          |                          ^~~~~~~~~
>> >>    ../hw/ppc/spapr_drc.c:1155:14: note: shadowed declaration is here
>> >>     1155 |     uint32_t drc_index;
>> >>          |              ^~~~~~~~~
>> >>
>> >> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> >> ---
>> >>   hw/ppc/spapr_drc.c | 2 --
>> >>   1 file changed, 2 deletions(-)
>> >>
>> >> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
>> >> index b5c400a94d1c..843e318312d3 100644
>> >> --- a/hw/ppc/spapr_drc.c
>> >> +++ b/hw/ppc/spapr_drc.c
>> >> @@ -1237,8 +1237,6 @@ static void
>> rtas_ibm_configure_connector(PowerPCCPU *cpu,
>> >>           case FDT_END_NODE:
>> >>               drc->ccs_depth--;
>> >>               if (drc->ccs_depth == 0) {
>> >> -                uint32_t drc_index = spapr_drc_index(drc);
>> >> -
>> > I guess you only wanted to remove re-declaration part. Assigning the
>>
>> value returned by this function doesnt seem to happen before.
>>
>> drc_index is assigned at the top of this large routine with :
>>
>>      drc_index = rtas_ld(wa_addr, 0);
>>      drc = spapr_drc_by_index(drc_index);
>>
>>
>> So, the extra local variable 'drc_index' is simply redundant because
>> there are no reason for it to change. The drc object is the same AFAICT.
>> Correct ? I should have explained that better in the commit log.
>>
>
> Okay, since both routines were implemented differently, I wasn't sure about
> the impact of reassignment. Better commit log is always welcome.

Do you expect a respin?  If not, would you like to give your R-by?
Re: [PATCH 6/8] spapr/drc: Clean up local variable shadowing in rtas_ibm_configure_connector()
Posted by Harsh Prateek Bora 7 months, 4 weeks ago

On 9/29/23 11:04, Markus Armbruster wrote:
> Harsh Prateek Bora <harsh.prateek.bora@gmail.com> writes:
> 
>> On Tue, 19 Sept, 2023, 5:33 pm Cédric Le Goater, <clg@kaod.org> wrote:
>>
>>> On 9/19/23 10:29, Harsh Prateek Bora wrote:
>>>>
>>>>
>>>> On 9/18/23 20:28, Cédric Le Goater wrote:
>>>>> Remove extra 'drc_index' variable to avoid this warning :
>>>>>
>>>>>     ../hw/ppc/spapr_drc.c: In function ‘rtas_ibm_configure_connector’:
>>>>>     ../hw/ppc/spapr_drc.c:1240:26: warning: declaration of ‘drc_index’
>>> shadows a previous local [-Wshadow=compatible-local]
>>>>>      1240 |                 uint32_t drc_index = spapr_drc_index(drc);
>>>>>           |                          ^~~~~~~~~
>>>>>     ../hw/ppc/spapr_drc.c:1155:14: note: shadowed declaration is here
>>>>>      1155 |     uint32_t drc_index;
>>>>>           |              ^~~~~~~~~
>>>>>
>>>>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>>>>> ---
>>>>>    hw/ppc/spapr_drc.c | 2 --
>>>>>    1 file changed, 2 deletions(-)
>>>>>
>>>>> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
>>>>> index b5c400a94d1c..843e318312d3 100644
>>>>> --- a/hw/ppc/spapr_drc.c
>>>>> +++ b/hw/ppc/spapr_drc.c
>>>>> @@ -1237,8 +1237,6 @@ static void
>>> rtas_ibm_configure_connector(PowerPCCPU *cpu,
>>>>>            case FDT_END_NODE:
>>>>>                drc->ccs_depth--;
>>>>>                if (drc->ccs_depth == 0) {
>>>>> -                uint32_t drc_index = spapr_drc_index(drc);
>>>>> -
>>>> I guess you only wanted to remove re-declaration part. Assigning the
>>>
>>> value returned by this function doesnt seem to happen before.
>>>
>>> drc_index is assigned at the top of this large routine with :
>>>
>>>       drc_index = rtas_ld(wa_addr, 0);
>>>       drc = spapr_drc_by_index(drc_index);
>>>
>>>
>>> So, the extra local variable 'drc_index' is simply redundant because
>>> there are no reason for it to change. The drc object is the same AFAICT.
>>> Correct ? I should have explained that better in the commit log.
>>>
>>
>> Okay, since both routines were implemented differently, I wasn't sure about
>> the impact of reassignment. Better commit log is always welcome.
> 
> Do you expect a respin?  If not, would you like to give your R-by?
> 
Oh sure,
Reviewed-by: Harsh Prateek Bora <harshpb@linux.ibm.com>