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

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

[Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 19 Jul 2024 16:12:51 +0200

Adjust the colour selection so that a bit of duplicate code can be avoided
in this function implementation.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 tools/perf/builtin-trace.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
index 8449f2beb54d..e29ae5cb95b0 100644
--- a/tools/perf/builtin-trace.c
+++ 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);
+
 	return printed + fprintf(fp, "): ");
 }

--
2.45.2

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

[Christian Heusel]

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

On 24/07/19 04:17PM, Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Fri, 19 Jul 2024 16:12:51 +0200
> 
> Adjust the colour selection so that a bit of duplicate code can be avoided
> in this function implementation.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  tools/perf/builtin-trace.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
> index 8449f2beb54d..e29ae5cb95b0 100644
> --- a/tools/perf/builtin-trace.c
> +++ 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? 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.

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

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

[Markus Elfring]

…
>> +++ 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?

Regards,
Markus

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

[Christian Heusel]

[-- 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 --]