| Message ID | 20200321114615.5360-6-philmd@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | misc: Trivial static code analyzer fixes | expand |
On Sat, 21 Mar 2020, Philippe Mathieu-Daudé wrote: > Fix warning reported by Clang static code analyzer: > > CC hw/ide/sii3112.o > hw/ide/sii3112.c:204:9: warning: Value stored to 'val' is never read > val = 0; > ^ ~ > > Reported-by: Clang Static Analyzer > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > hw/ide/sii3112.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/ide/sii3112.c b/hw/ide/sii3112.c > index 06605d7af2..36f1905ddb 100644 > --- a/hw/ide/sii3112.c > +++ b/hw/ide/sii3112.c > @@ -125,7 +125,7 @@ static uint64_t sii3112_reg_read(void *opaque, hwaddr addr, > val = (uint32_t)d->regs[1].sien << 16; > break; > default: > - val = 0; > + break; > } > trace_sii3112_read(size, addr, val); > return val; Value is clearly used in trace and return so don't really get why the compiler complains here. Looks like wrong warning to me. It's true however that since val is init to 0 at the beginning this assignment is not strictily needed and this should work as well, so Reviewed-by: BALATON Zoltan <balaton@eik.bme.hu> Regards, BALATON Zoltan
On Saturday, March 21, 2020, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > Fix warning reported by Clang static code analyzer: > > CC hw/ide/sii3112.o > hw/ide/sii3112.c:204:9: warning: Value stored to 'val' is never read > val = 0; > ^ ~ > > Reported-by: Clang Static Analyzer > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > hw/ide/sii3112.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/ide/sii3112.c b/hw/ide/sii3112.c > index 06605d7af2..36f1905ddb 100644 > --- a/hw/ide/sii3112.c > +++ b/hw/ide/sii3112.c > @@ -125,7 +125,7 @@ static uint64_t sii3112_reg_read(void *opaque, hwaddr > addr, > val = (uint32_t)d->regs[1].sien << 16; > break; > default: > - val = 0; > + break; There is another function in the same file, having a similar switch statement. There is no warning for that second tunction, since "val" is its parameter, not a local varioble, like is the case here. This means that the proposed change introduces inconsistency between two functions, therefore it is better to remove the initialization of "val" to 0, than to remove this line in "default" section. Regards, Aleksandar > } > trace_sii3112_read(size, addr, val); > return val; > -- > 2.21.1 > > >
On Sat, 21 Mar 2020, Aleksandar Markovic wrote: > On Saturday, March 21, 2020, Philippe Mathieu-Daudé <philmd@redhat.com> > wrote: > >> Fix warning reported by Clang static code analyzer: >> >> CC hw/ide/sii3112.o >> hw/ide/sii3112.c:204:9: warning: Value stored to 'val' is never read >> val = 0; >> ^ ~ >> >> Reported-by: Clang Static Analyzer >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> >> --- >> hw/ide/sii3112.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/hw/ide/sii3112.c b/hw/ide/sii3112.c >> index 06605d7af2..36f1905ddb 100644 >> --- a/hw/ide/sii3112.c >> +++ b/hw/ide/sii3112.c >> @@ -125,7 +125,7 @@ static uint64_t sii3112_reg_read(void *opaque, hwaddr >> addr, >> val = (uint32_t)d->regs[1].sien << 16; >> break; >> default: >> - val = 0; >> + break; > > > There is another function in the same file, having a similar switch > statement. There is no warning for that second tunction, since "val" is its > parameter, not a local varioble, like is the case here. This means that the > proposed change introduces inconsistency between two functions, therefore > it is better to remove the initialization of "val" to 0, than to remove > this line in "default" section. Oh, actually I think the warning was for that statement not for the one patched as it makes more sense there where val is assigned in void sii3112_reg_write() where it's indeed not used so maybe that was meant to be patched instead? Regards, BALATON Zoltan
On 3/21/20 2:39 PM, Aleksandar Markovic wrote: > > > On Saturday, March 21, 2020, Philippe Mathieu-Daudé <philmd@redhat.com > <mailto:philmd@redhat.com>> wrote: > > Fix warning reported by Clang static code analyzer: > > CC hw/ide/sii3112.o > hw/ide/sii3112.c:204:9: warning: Value stored to 'val' is never read > val = 0; > ^ ~ > > Reported-by: Clang Static Analyzer > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com > <mailto:philmd@redhat.com>> > --- > hw/ide/sii3112.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/ide/sii3112.c b/hw/ide/sii3112.c > index 06605d7af2..36f1905ddb 100644 > --- a/hw/ide/sii3112.c > +++ b/hw/ide/sii3112.c > @@ -125,7 +125,7 @@ static uint64_t sii3112_reg_read(void *opaque, > hwaddr addr, > val = (uint32_t)d->regs[1].sien << 16; > break; > default: > - val = 0; > + break; > > > There is another function in the same file, having a similar switch > statement. There is no warning for that second tunction, since "val" is > its parameter, not a local varioble, like is the case here. This means > that the proposed change introduces inconsistency between two functions, > therefore it is better to remove the initialization of "val" to 0, than > to remove this line in "default" section. No clue why there is no warnings emitted for sii3112_reg_write()... Do you mind sending a patch? > > Regards, > Aleksandar > > } > trace_sii3112_read(size, addr, val); > return val; > -- > 2.21.1 > >
On 3/21/20 3:12 PM, BALATON Zoltan wrote: > On Sat, 21 Mar 2020, Aleksandar Markovic wrote: >> On Saturday, March 21, 2020, Philippe Mathieu-Daudé <philmd@redhat.com> >> wrote: >> >>> Fix warning reported by Clang static code analyzer: >>> >>> CC hw/ide/sii3112.o >>> hw/ide/sii3112.c:204:9: warning: Value stored to 'val' is never read >>> val = 0; >>> ^ ~ >>> >>> Reported-by: Clang Static Analyzer >>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> >>> --- >>> hw/ide/sii3112.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/hw/ide/sii3112.c b/hw/ide/sii3112.c >>> index 06605d7af2..36f1905ddb 100644 >>> --- a/hw/ide/sii3112.c >>> +++ b/hw/ide/sii3112.c >>> @@ -125,7 +125,7 @@ static uint64_t sii3112_reg_read(void *opaque, >>> hwaddr >>> addr, >>> val = (uint32_t)d->regs[1].sien << 16; >>> break; >>> default: >>> - val = 0; >>> + break; >> >> >> There is another function in the same file, having a similar switch >> statement. There is no warning for that second tunction, since "val" >> is its >> parameter, not a local varioble, like is the case here. This means >> that the >> proposed change introduces inconsistency between two functions, therefore >> it is better to remove the initialization of "val" to 0, than to remove >> this line in "default" section. > > Oh, actually I think the warning was for that statement not for the one > patched as it makes more sense there where val is assigned in void > sii3112_reg_write() where it's indeed not used so maybe that was meant > to be patched instead? Ah, the warning is for hw/ide/sii3112.c:204 but I incorrectly patched hw/ide/sii3112.c:128 =) > > Regards, > BALATON Zoltan
On Sat, 21 Mar 2020, Philippe Mathieu-Daudé wrote: > On 3/21/20 3:12 PM, BALATON Zoltan wrote: >> On Sat, 21 Mar 2020, Aleksandar Markovic wrote: >>> On Saturday, March 21, 2020, Philippe Mathieu-Daudé <philmd@redhat.com> >>> wrote: >>> >>>> Fix warning reported by Clang static code analyzer: >>>> >>>> CC hw/ide/sii3112.o >>>> hw/ide/sii3112.c:204:9: warning: Value stored to 'val' is never read >>>> val = 0; >>>> ^ ~ >>>> >>>> Reported-by: Clang Static Analyzer >>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> >>>> --- >>>> hw/ide/sii3112.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/hw/ide/sii3112.c b/hw/ide/sii3112.c >>>> index 06605d7af2..36f1905ddb 100644 >>>> --- a/hw/ide/sii3112.c >>>> +++ b/hw/ide/sii3112.c >>>> @@ -125,7 +125,7 @@ static uint64_t sii3112_reg_read(void *opaque, hwaddr >>>> addr, >>>> val = (uint32_t)d->regs[1].sien << 16; >>>> break; >>>> default: >>>> - val = 0; >>>> + break; >>> >>> >>> There is another function in the same file, having a similar switch >>> statement. There is no warning for that second tunction, since "val" is >>> its >>> parameter, not a local varioble, like is the case here. This means that >>> the >>> proposed change introduces inconsistency between two functions, therefore >>> it is better to remove the initialization of "val" to 0, than to remove >>> this line in "default" section. >> >> Oh, actually I think the warning was for that statement not for the one >> patched as it makes more sense there where val is assigned in void >> sii3112_reg_write() where it's indeed not used so maybe that was meant to >> be patched instead? > > Ah, the warning is for hw/ide/sii3112.c:204 but I incorrectly patched > hw/ide/sii3112.c:128 =) Now you may patch both to replace val = 0 with break to keep symmetry. My Reviewed-by stands (even if apparently not much use). Thanks for Aleksandar for spotting it. Regards, BALATON Zoltan
On 3/21/20 3:14 PM, Philippe Mathieu-Daudé wrote: > On 3/21/20 2:39 PM, Aleksandar Markovic wrote: >> >> >> On Saturday, March 21, 2020, Philippe Mathieu-Daudé <philmd@redhat.com >> <mailto:philmd@redhat.com>> wrote: >> >> Fix warning reported by Clang static code analyzer: >> >> CC hw/ide/sii3112.o >> hw/ide/sii3112.c:204:9: warning: Value stored to 'val' is never >> read >> val = 0; >> ^ ~ >> >> Reported-by: Clang Static Analyzer >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com >> <mailto:philmd@redhat.com>> >> --- >> hw/ide/sii3112.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/hw/ide/sii3112.c b/hw/ide/sii3112.c >> index 06605d7af2..36f1905ddb 100644 >> --- a/hw/ide/sii3112.c >> +++ b/hw/ide/sii3112.c >> @@ -125,7 +125,7 @@ static uint64_t sii3112_reg_read(void *opaque, >> hwaddr addr, >> val = (uint32_t)d->regs[1].sien << 16; >> break; >> default: >> - val = 0; >> + break; >> >> >> There is another function in the same file, having a similar switch >> statement. There is no warning for that second tunction, since "val" >> is its parameter, not a local varioble, like is the case here. This >> means that the proposed change introduces inconsistency between two >> functions, therefore it is better to remove the initialization of >> "val" to 0, than to remove this line in "default" section. > > No clue why there is no warnings emitted for sii3112_reg_write()... > > Do you mind sending a patch? Don't worry I'll follow up. > >> >> Regards, >> Aleksandar >> >> } >> trace_sii3112_read(size, addr, val); >> return val; >> -- 2.21.1 >> >>
diff --git a/hw/ide/sii3112.c b/hw/ide/sii3112.c index 06605d7af2..36f1905ddb 100644 --- a/hw/ide/sii3112.c +++ b/hw/ide/sii3112.c @@ -125,7 +125,7 @@ static uint64_t sii3112_reg_read(void *opaque, hwaddr addr, val = (uint32_t)d->regs[1].sien << 16; break; default: - val = 0; + break; } trace_sii3112_read(size, addr, val); return val;
Fix warning reported by Clang static code analyzer: CC hw/ide/sii3112.o hw/ide/sii3112.c:204:9: warning: Value stored to 'val' is never read val = 0; ^ ~ Reported-by: Clang Static Analyzer Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> --- hw/ide/sii3112.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)