Re: [PATCH v8 5/7] progress.c: add temporary variable from progress struct
- From
Johannes Altmanninger <aclopte@gmail.com>
- Date
- Dec 28, 2021, 16:13 UTC
- Message-ID
- <20211228161350.mzsubl3vensst2hu@gmail.com>
- In-Reply-To
- <patch-v8-5.7-72a31bd7191-20211228T150728Z-avarab@gmail.com>
On Tue, Dec 28, 2021 at 04:19:01PM +0100, Ævar Arnfjörð Bjarmason wrote:
> Since 98a13647408 (trace2: log progress time and throughput, > 2020-05-12) stop_progress() dereferences a "struct progress **" > parameter in several places. Extract a dereferenced variable (like in > stop_progress_msg()) to reduce clutter and make it clearer who needs
The "(like in stop_progress_msg())" can probably go because you explain the added consistency in the next paragraph.
Show 6 quoted lines
> to write to this parameter. > > Now instead of using "*p_progress" several times in stop_progress() we > check it once for NULL and then use a dereferenced "progress" variable > thereafter. This continues the same pattern used in the above > stop_progress() function, see ac900fddb7f (progress: don't dereference
"above stop_progress" should be "below stop_progress_msg", because stop_progress is the one you're modifying?
Show 16 quoted lines
> before checking for NULL, 2020-08-10).
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
> progress.c | 14 ++++++++------
> 1 file changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/progress.c b/progress.c
> index 680c6a8bf93..688749648be 100644
> --- a/progress.c
> +++ b/progress.c
> @@ -319,21 +319,23 @@ static void finish_if_sparse(struct progress *progress)
>
> void stop_progress(struct progress **p_progress)
> {
> + struct progress *progress;nit: in stop_progress_msg we have a blank line here, the inconsistency is mildly surprising
Show 28 quoted lines
> if (!p_progress)
> BUG("don't provide NULL to stop_progress");
> + progress = *p_progress;
>
> - finish_if_sparse(*p_progress);
> + finish_if_sparse(progress);
>
> - if (*p_progress) {
> + if (progress) {
> trace2_data_intmax("progress", the_repository, "total_objects",
> - (*p_progress)->total);
> + progress->total);
>
> - if ((*p_progress)->throughput)
> + if (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);
> }
>
> stop_progress_msg(p_progress, _("done"));
> --
> 2.34.1.1257.g2af47340c7b
>