Thread (75 messages) flat view 75 messages, 8 authors, 2022-01-27

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

From: René Scharfe <hidden>
Date: 2021-12-28 16:05:37

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é
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help