Re: [PATCH] perf trace: Avoid duplicate code in fprintf_duration()

[Christian Heusel <christian@heusel.eu>]

[-- Attachment #1: Type: text/plain, Size: 1508 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]