| Message ID | 20230918145850.241074-7-clg@kaod.org |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | ppc: Clean up local variable shadowing | expand |
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;
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;
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; > > >
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?
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>
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;
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(-)