linux-kernel - Re: [PATCH] perf trace: Avoid duplicate code in fprintf

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <b48460e2-c54d-4390-89e3-6835c379eb01@heusel.eu>
Date: Sat, 20 Jul 2024 13:29:56 +0200
From: Christian Heusel <christian@...sel.eu>
To: Markus Elfring <Markus.Elfring@....de>
Cc: linux-perf-users@...r.kernel.org, kernel-janitors@...r.kernel.org, 
	Adrian Hunter <adrian.hunter@...el.com>, Alexander Shishkin <alexander.shishkin@...ux.intel.com>, 
	Arnaldo Carvalho de Melo <acme@...nel.org>, Ian Rogers <irogers@...gle.com>, Ingo Molnar <mingo@...hat.com>, 
	Jiri Olsa <jolsa@...nel.org>, Kan Liang <kan.liang@...ux.intel.com>, 
	Mark Rutland <mark.rutland@....com>, Namhyung Kim <namhyung@...nel.org>, 
	Peter Zijlstra <peterz@...radead.org>, LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] perf trace: Avoid duplicate code in fprintf_duration()

On 24/07/19 06:32PM, Markus Elfring wrote:
> …
> >> +++ b/tools/perf/builtin-trace.c
> >> @@ -1258,12 +1258,16 @@ static size_t fprintf_duration(unsigned long t, bool calculated, FILE *fp)
> >>
> >>  	if (!calculated)
> >>  		printed += fprintf(fp, "         ");
> >> -	else if (duration >= 1.0)
> >> -		printed += color_fprintf(fp, PERF_COLOR_RED, "%6.3f ms", duration);
> >> -	else if (duration >= 0.01)
> >> -		printed += color_fprintf(fp, PERF_COLOR_YELLOW, "%6.3f ms", duration);
> >>  	else
> >> -		printed += color_fprintf(fp, PERF_COLOR_NORMAL, "%6.3f ms", duration);
> >> +		printed += color_fprintf(fp,
> >> +					 (duration >= 1.0
> >> +					 ? PERF_COLOR_RED
> >> +					 : (duration >= 0.01
> >> +					   ? PERF_COLOR_YELLOW
> >> +					   : PERF_COLOR_NORMAL)),
> >> +					 "%6.3f ms",
> >> +					 duration);
> >
> > Why is this a desirable change?
> 
> I find it helpful to specify the affected function call only once
> in such an if branch.
> 
> 
> >                                 Folding the if-statements into the
> > ternary operator makes the code quite unreadable compared to what it was
> > like before and doesn't give any obvious improvement.
> 
> Do you prefer to store the result of the colour determination into another
> local variable so that it can be passed as a separate parameter?

No I think I prefer the current version of the code. But my judgement
does not matter much here, lets wait what the maintainers have to say
about this.

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)