| Message ID | patch-v3-07.10-cd2d27b1626-20211013T222329Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | progress: assert "global_progress" + test fixes / cleanup | expand |
On Thu, Oct 14, 2021 at 12:28:23AM +0200, Ævar Arnfjörð Bjarmason wrote: > 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... > 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
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); 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); } stop_progress_msg(p_progress, _("done"));
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(-)