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

From: Christian Heusel
Date: Sat Jul 20 2024 - 07:44:39 EST


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: signature.asc
Description: PGP signature