Re: [PATCH v8 5/7] progress.c: add temporary variable from progress struct

From
René Scharfe <hidden>
Date
2021-12-28 16:05:37
Reply to
Ævar Arnfjörð Bjarmason, 2021-12-28
Am 28.12.21 um 16:19 schrieb Ævar Arnfjörð Bjarmason:
quoted hunk ↗ jump to hunk
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
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
before checking for NULL, 2020-08-10).

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
 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;
 	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"));
This patch is trivially correct, but I wonder why all that code is here
instead of in stop_progress_msg().  I would expect stop_progress() to be
a thin wrapper that just provides a default message, but actually it
handles sparse progress and tracing.  Isn't both necessary even with a
custom message?

In any case, moving the code there becomes easier after this patch.

René
Thread (75 messages) 75 messages, 8 authors, last reply 2022-01-27
Keyboard shortcuts
hback out one level
jnext message in thread / next series-diff position
kprevious message in thread / previous series-diff position
ldrill in
Escclose help / fold thread tree
?toggle this help