| Message ID | 20230828214739.GF3831137@coredump.intra.peff.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | YAUPS: Yet Another Unused Parameter Series | expand |
On Mon, Aug 28, 2023 at 05:47:39PM -0400, Jeff King wrote: > The compute_generation_info code uses function pointers to abstract the > get/set generation operations. Some callers don't need the extra void > data pointer, which should be annotated to appease -Wunused-parameter. I think just the callbacks initialized by compute_topological_levels() (i.e, get_topo_level() and set_topo_level()) care about the ctx parameter. I think that we can go a bit further, though. The other caller in compute_generation_numbers() assigns data to the argument ctx it takes in, but neither of its callbacks get_generation_from_graph_data() and set_generation_v2() use the data parameter, as is implied by the the existence of this patch. So I think that we could drop the assignment to data entirely like so, applied on top of this patch: --- 8< --- diff --git a/commit-graph.c b/commit-graph.c index 0aa1640d15..eb3e27b720 100644 --- a/commit-graph.c +++ b/commit-graph.c @@ -1587,7 +1587,6 @@ static void compute_generation_numbers(struct write_commit_graph_context *ctx) .commits = &ctx->commits, .get_generation = get_generation_from_graph_data, .set_generation = set_generation_v2, - .data = ctx, }; if (ctx->report_progress) --- >8 --- > Signed-off-by: Jeff King <peff@peff.net> > --- > commit-graph.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) The rest looks obviously correct to me, thanks. Thanks, Taylor
On Mon, Aug 28, 2023 at 07:32:59PM -0400, Taylor Blau wrote: > On Mon, Aug 28, 2023 at 05:47:39PM -0400, Jeff King wrote: > > The compute_generation_info code uses function pointers to abstract the > > get/set generation operations. Some callers don't need the extra void > > data pointer, which should be annotated to appease -Wunused-parameter. > > I think just the callbacks initialized by compute_topological_levels() > (i.e, get_topo_level() and set_topo_level()) care about the ctx > parameter. > > I think that we can go a bit further, though. The other caller in > compute_generation_numbers() assigns data to the argument ctx it takes > in, but neither of its callbacks get_generation_from_graph_data() and > set_generation_v2() use the data parameter, as is implied by the the > existence of this patch. > > So I think that we could drop the assignment to data entirely like so, > applied on top of this patch: > > --- 8< --- > diff --git a/commit-graph.c b/commit-graph.c > index 0aa1640d15..eb3e27b720 100644 > --- a/commit-graph.c > +++ b/commit-graph.c > @@ -1587,7 +1587,6 @@ static void compute_generation_numbers(struct write_commit_graph_context *ctx) > .commits = &ctx->commits, > .get_generation = get_generation_from_graph_data, > .set_generation = set_generation_v2, > - .data = ctx, > }; > > if (ctx->report_progress) > --- >8 --- Yeah, I didn't dig too much in the caller. I could see an argument for leaving it, in that it might be the "least surprise" for somebody who later wants to look at the ctx variable from those callbacks. But since all they get is a void pointer, anybody writing that code would have to go back to the caller to see what is passed in as "data" anyway. So I think it is safe to add this cleanup. I'm going to re-roll to simplify the first patch a bit, so I'll add this in as well. Thanks. -Peff
diff --git a/commit-graph.c b/commit-graph.c index 0aa1640d15..73a539495b 100644 --- a/commit-graph.c +++ b/commit-graph.c @@ -1568,12 +1568,14 @@ static void compute_topological_levels(struct write_commit_graph_context *ctx) stop_progress(&ctx->progress); } -static timestamp_t get_generation_from_graph_data(struct commit *c, void *data) +static timestamp_t get_generation_from_graph_data(struct commit *c, + void *data UNUSED) { return commit_graph_data_at(c)->generation; } -static void set_generation_v2(struct commit *c, timestamp_t t, void *data) +static void set_generation_v2(struct commit *c, timestamp_t t, + void *data UNUSED) { struct commit_graph_data *g = commit_graph_data_at(c); g->generation = t; @@ -1616,7 +1618,7 @@ static void compute_generation_numbers(struct write_commit_graph_context *ctx) } static void set_generation_in_graph_data(struct commit *c, timestamp_t t, - void *data) + void *data UNUSED) { commit_graph_data_at(c)->generation = t; }
The compute_generation_info code uses function pointers to abstract the get/set generation operations. Some callers don't need the extra void data pointer, which should be annotated to appease -Wunused-parameter. Signed-off-by: Jeff King <peff@peff.net> --- commit-graph.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)