Re: [PATCH v3 07/10] progress.c: add temporary variable from progress struct

From: Taylor Blau <me@ttaylorr.com>
Date: Oct 22, 2021, 22:37 UTC
Message-ID: <YXM9KVUousJXgRup@nand.local>
In-Reply-To: <patch-v3-07.10-cd2d27b1626-20211013T222329Z-avarab@gmail.com>

On Thu, Oct 14, 2021 at 12:28:23AM +0200, Ævar Arnfjörð Bjarmason wrote:

Show 18 quoted lines

> Add a temporary "progress" variable for the dereferenced p_progress
> pointer to a "struct progress *". Before 98a13647408 (trace2: log
> progress time and throughput, 2020-05-12) we didn't dereference
> "p_progress" in this function, now that we do it's easier to read the
> code if we work with a "progress" struct pointer like everywhere else,
> instead of a pointer to a pointer.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  progress.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/progress.c b/progress.c
> index 7fcc513717a..b9369e9a264 100644
> --- a/progress.c
> +++ b/progress.c
> @@ -329,15 +329,16 @@ void stop_progress(struct progress **p_progress)
>  	finish_if_sparse(*p_progress);

This spot also wants to dereference p_progress, but doesn't access any of the fields by dereferencing the second pointer. So this could certainly have passed `progress` (had you scoped its declaration to the whole function).

I don't think it hurts readability, and I definitely don't have a strong opinion about it. Just an idle thought while reading this patch...

Show 13 quoted lines

>  	if (*p_progress) {
> +		struct progress *progress = *p_progress;
>  		trace2_data_intmax("progress", the_repository, "total_objects",
>  				   (*p_progress)->total);
>
>  		if ((*p_progress)->throughput)
>  			trace2_data_intmax("progress", the_repository,
>  					   "total_bytes",
> -					   (*p_progress)->throughput->curr_total);
> +					   progress->throughput->curr_total);
>
> -		trace2_region_leave("progress", (*p_progress)->title, the_repository);
> +		trace2_region_leave("progress", progress->title, the_repository);

This all looks much better to me. Thanks.

Thanks, Taylor