On Mon, Jun 21, 2021 at 03:24:47AM +0200, Ævar Arnfjörð Bjarmason wrote:
> > [...]
> > @@ -320,6 +321,22 @@ void stop_progress(struct progress **p_progress)
> > {
> > finish_if_sparse(*p_progress);
> >
> > + if (p_progress && *p_progress) {
> > + trace2_data_intmax("progress", the_repository, "total_objects",
> > + (*p_progress)->total);
>
> We start progress bars for various things in git, yet the trace2 data
> calls every such progress bar with a total "total_objects", even though
> we may not be counting anything to do with objects.
>
> Wouldn't simply s/total_objects/total/ make more sense here, do you rely
> on the name of the current key?
Yeah, I think that the latter of just "total" makes more sense here. I
was going to comment on the fact that "(*p_progress)->total" could be
written simply as "*p_progress->total", but I'm (a) not sure that I
actually prefer the latter to the former, and (b) I find that kind of
style comment generally useless.
But it may make sense to sidestep the whole thing and have a "struct
progress *progress = *p_progress" (that is assigned after we check
p_progress to make sure it's non-NULL) like in stop_progress_msg, which
would clean up a lot of this.
--- 8< ---
Subject: [PATCH] progress.c: avoid repeatedly dereferencing p_progress
stop_progress() takes a double-pointer to a "progress" struct, and
dereferences it twice in each use except one (checking whether
*p_progress is NULL or not).
Mirror the implementation of stop_progress_msg() below by having a local
single-pointer to a progress struct (which is the dereference of
p_progress) to avoid repeatedly dereferencing it.
Signed-off-by: Taylor Blau <me@xxxxxxxxxxxx>
---
progress.c | 16 ++++++++++------
1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/progress.c b/progress.c
index 680c6a8bf9..390c76b22a 100644
--- a/progress.c
+++ b/progress.c
@@ -319,21 +319,25 @@ 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");
- finish_if_sparse(*p_progress);
+ progress = *p_progress;
- if (*p_progress) {
+ finish_if_sparse(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.31.1.163.ga65ce7f831
