| Message ID | 20240828040944.GA4005021@coredump.intra.peff.net (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | e49d2472d2320d370b34c7fcdde933660c47968b |
| Headers | show |
| Series | reftable: mark unused parameters in empty iterator functions | expand |
On Wed, Aug 28, 2024 at 12:09:44AM -0400, Jeff King wrote: > These unused parameters were marked in a68ec8683a (reftable: mark unused > parameters in virtual functions, 2024-08-17), but the functions were > moved to a new file in a parallel branch via f2406c81b9 > (reftable/generic: move generic iterator code into iterator interface, > 2024-08-22). > > Signed-off-by: Jeff King <peff@peff.net> > --- > This should go on top of ps/reftable-drop-generic. Arguably this could > have been done as part of the conflict resolution when merging into next > alongside jk/mark-unused-parameters, but at this point I think a > separate patch is the best way forward. > > reftable/iter.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/reftable/iter.c b/reftable/iter.c > index 97a4642ed5..1d99fe4f7d 100644 > --- a/reftable/iter.c > +++ b/reftable/iter.c > @@ -25,17 +25,17 @@ int iterator_next(struct reftable_iterator *it, struct reftable_record *rec) > return it->ops->next(it->iter_arg, rec); > } > > -static int empty_iterator_seek(void *arg, struct reftable_record *want) > +static int empty_iterator_seek(void *arg UNUSED, struct reftable_record *want UNUSED) > { > return 0; > } > > -static int empty_iterator_next(void *arg, struct reftable_record *rec) > +static int empty_iterator_next(void *arg UNUSED, struct reftable_record *rec UNUSED) > { > return 1; > } > > -static void empty_iterator_close(void *arg) > +static void empty_iterator_close(void *arg UNUSED) > { > } These changes look obviously correct to me, thanks! Patrick
Jeff King <peff@peff.net> writes: > This should go on top of ps/reftable-drop-generic. Arguably this could > have been done as part of the conflict resolution when merging into next > alongside jk/mark-unused-parameters, but at this point I think a > separate patch is the best way forward. As marking with UNUSED used to be optional before -Wno-unused-param got removed, I agree that it is better to apply this on top to be explicit, rather than burying it in an evil merge with a topic that marked other unrelated parameters as UNUSED.
On Wed, Aug 28, 2024 at 10:12:22AM -0700, Junio C Hamano wrote: > Jeff King <peff@peff.net> writes: > > > This should go on top of ps/reftable-drop-generic. Arguably this could > > have been done as part of the conflict resolution when merging into next > > alongside jk/mark-unused-parameters, but at this point I think a > > separate patch is the best way forward. > > As marking with UNUSED used to be optional before -Wno-unused-param > got removed, I agree that it is better to apply this on top to be > explicit, rather than burying it in an evil merge with a topic that > marked other unrelated parameters as UNUSED. [warning: philosophical rambling ahead] The reason I mentioned the merge here is that I think you could argue this is a mis-merge that already happened. Forget for a moment the recent series that makes UNUSED non-optional, and consider the merge of jk/mark-unused-parameters and ps/reftable-drop-generic. The former updated code in reftable/generic.c, and the latter removed that file entirely, so there's a conflict. And it's tempting to say "ok, we don't care about this code anymore, so take the deletion", which is what your merge did. But the code in question was actually moved in that series, via f2406c81b9 (reftable/generic: move generic iterator code into iterator interface, 2024-08-22). So I think the correct resolution for the merge is to move those updates along with the code into the new file; otherwise we are accidentally reverting part of what jk/mark-unused-parameters did. That said, I think you can get pretty philosophical here. Did the reftable topic move the code, or did it delete some old code and add some new code that needed the same change? :) I also think it's pretty hard to notice these kinds of resolutions in practice. There's a conflict at the point of deletion, but there's nothing in the merge workflow that tells you "btw, this code is now over here, so you should port the modifications from the side branch over". Interestingly, I think a rebase of one topic onto the other might have made it more clear, since the code movement happened in its own step (which would make what was going on more obvious). I guess Michael Haggerty's "imerge" would probably show something similar (actually, I just tried it, and it indeed hones in on f2406c81 and 4695c3f3 as the source of the conflict). Anyway, I am not proposing to do anything different. _If_ we considered the merge of the two topics that is in next to be an incorrect resolution, we could repair that when merging to master. But I think doing so is complicated. And certainly the philosophy of "if something is tricky, try to be as explicit as possible" seems like a good one here. Mostly I just found it kind of an interesting case. :) -Peff
diff --git a/reftable/iter.c b/reftable/iter.c index 97a4642ed5..1d99fe4f7d 100644 --- a/reftable/iter.c +++ b/reftable/iter.c @@ -25,17 +25,17 @@ int iterator_next(struct reftable_iterator *it, struct reftable_record *rec) return it->ops->next(it->iter_arg, rec); } -static int empty_iterator_seek(void *arg, struct reftable_record *want) +static int empty_iterator_seek(void *arg UNUSED, struct reftable_record *want UNUSED) { return 0; } -static int empty_iterator_next(void *arg, struct reftable_record *rec) +static int empty_iterator_next(void *arg UNUSED, struct reftable_record *rec UNUSED) { return 1; } -static void empty_iterator_close(void *arg) +static void empty_iterator_close(void *arg UNUSED) { }
These unused parameters were marked in a68ec8683a (reftable: mark unused parameters in virtual functions, 2024-08-17), but the functions were moved to a new file in a parallel branch via f2406c81b9 (reftable/generic: move generic iterator code into iterator interface, 2024-08-22). Signed-off-by: Jeff King <peff@peff.net> --- This should go on top of ps/reftable-drop-generic. Arguably this could have been done as part of the conflict resolution when merging into next alongside jk/mark-unused-parameters, but at this point I think a separate patch is the best way forward. reftable/iter.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)