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
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>
© 2016 - 2024 Red Hat, Inc.