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
