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

From: Arnaldo Carvalho de Melo
Date: Mon Nov 11 2024 - 13:07:10 EST


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@xxxxxxxxxxxxxxxxxxxx>
> > > ---
> > > 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
> > >
> > >