Re: [PATCH] perf tools: Remove redundant variable assignment

[Arnaldo Carvalho de Melo <acme@kernel.org>]

On Mon, Nov 11, 2024 at 02:51:28PM -0300, Arnaldo Carvalho de Melo wrote:
> On Mon, Nov 11, 2024 at 02:42:46PM -0300, Arnaldo Carvalho de Melo wrote:
> > On Mon, Nov 11, 2024 at 04:27:13PM +0800, Luo Yifan wrote:
> > > This patch makes a minor change that removes the redundant assignment
> > > to the variable ret, simplifying the code.
> > 
> > Thanks, applied to perf-tools-next,
> 
> Are you build testing these patches?
> 
>  GEN     perf-archive
>   CC      /tmp/build/perf-tools-next/libsubcmd/sigchain.o
>   GEN     perf-iostat
>   INSTALL libbpf_headers
>   LD      /tmp/build/perf-tools-next/libsubcmd/libsubcmd-in.o
>   AR      /tmp/build/perf-tools-next/libsubcmd/libsubcmd.a
> jvmti/jvmti_agent.c: In function ‘jvmti_write_code’:
> jvmti/jvmti_agent.c:366:13: error: variable ‘ret’ set but not used [-Werror=unused-but-set-variable]
>   366 |         int ret = -1;
>       |             ^~~
> cc1: all warnings being treated as errors
> make[3]: *** [/home/acme/git/perf-tools-next/tools/build/Makefile.build:106: /tmp/build/perf-tools-next/jvmti/jvmti_agent.o] Error 1
> make[2]: *** [Makefile.perf:936: /tmp/build/perf-tools-next/jvmti/jvmti-in.o] Error 2
> make[2]: *** Waiting for unfinished jobs....
>   CC      /tmp/build/perf-tools-next/util/header.o
>   LD      /tmp/build/perf-tools-next/util/perf-util-in.o
>   LD      /tmp/build/perf-tools-next/perf-util-in.o
> make[1]: *** [Makefile.perf:292: sub-make] Error 2
> make: *** [Makefile:119: install-bin] Error 2
> make: Leaving directory '/home/acme/git/perf-tools-next/tools/perf'
> ⬢ [acme@toolbox perf-tools-next]$
> 
> The original patch by Stephane has it right, that initial ret = -1 is
> used when there are other problems and the code goes to a label at the
> end, returning that -1.
> 
> But the code was changed later and problems were introduced, so you
> removed something simple at the end and somehow missed that it breaks
> the build (at least for me) and when I go look at the code, I see the
> other problems, so please take the time to try and investigate this and
> fix the 'ret' variable usage.
> 
> I'm removing this patch from my local tree.

Ok, some of the other functions use the label at the end + return ret
and looks nice, but the jvmti_write_code() one has this problem since
day one, just look at the other routines and fix this one, please.

- Arnaldo
 
> Thanks,
>  
> > - Arnaldo
> >  
> > > Signed-off-by: Luo Yifan <luoyifan@cmss.chinamobile.com>
> > > ---
> > >  tools/perf/jvmti/jvmti_agent.c | 4 +---
> > >  1 file changed, 1 insertion(+), 3 deletions(-)
> > > 
> > > diff --git a/tools/perf/jvmti/jvmti_agent.c b/tools/perf/jvmti/jvmti_agent.c
> > > index 526dcaf9f..751219143 100644
> > > --- a/tools/perf/jvmti/jvmti_agent.c
> > > +++ b/tools/perf/jvmti/jvmti_agent.c
> > > @@ -408,9 +408,7 @@ jvmti_write_code(void *agent, char const *sym,
> > >  
> > >  	funlockfile(fp);
> > >  
> > > -	ret = 0;
> > > -
> > > -	return ret;
> > > +	return 0;
> > >  }
> > >  
> > >  int
> > > -- 
> > > 2.27.0
> > > 
> > >