{"thread":{"id":"62015","subject":"[PATCH] reftable: mark unused parameters in empty iterator functions","startedAt":"2024-08-28T04:09:45Z","lastAt":"2024-08-28T18:10:04Z","messageCount":4,"participants":["Jeff King","Patrick Steinhardt","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"501752","messageId":"20240828040944.GA4005021@coredump.intra.peff.net","threadId":"62015","inReplyTo":null,"subject":"[PATCH] reftable: mark unused parameters in empty iterator functions","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-08-28T04:09:44Z","receivedAt":"2024-08-28T04:09:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"These unused parameters were marked in a68ec8683a (reftable: mark unused\nparameters in virtual functions, 2024-08-17), but the functions were\nmoved to a new file in a parallel branch via f2406c81b9\n(reftable/generic: move generic iterator code into iterator interface,\n2024-08-22).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThis should go on top of ps/reftable-drop-generic. Arguably this could\nhave been done as part of the conflict resolution when merging into next\nalongside jk/mark-unused-parameters, but at this point I think a\nseparate patch is the best way forward.\n\n reftable/iter.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/reftable/iter.c b/reftable/iter.c\nindex 97a4642ed5..1d99fe4f7d 100644\n--- a/reftable/iter.c\n+++ b/reftable/iter.c\n@@ -25,17 +25,17 @@ int iterator_next(struct reftable_iterator *it, struct reftable_record *rec)\n \treturn it->ops->next(it->iter_arg, rec);\n }\n \n-static int empty_iterator_seek(void *arg, struct reftable_record *want)\n+static int empty_iterator_seek(void *arg UNUSED, struct reftable_record *want UNUSED)\n {\n \treturn 0;\n }\n \n-static int empty_iterator_next(void *arg, struct reftable_record *rec)\n+static int empty_iterator_next(void *arg UNUSED, struct reftable_record *rec UNUSED)\n {\n \treturn 1;\n }\n \n-static void empty_iterator_close(void *arg)\n+static void empty_iterator_close(void *arg UNUSED)\n {\n }\n \n-- \n2.46.0.754.g24c813f009\n"},{"id":"501755","messageId":"Zs7diDgkP33jG5W7@tanuki","threadId":"62015","inReplyTo":"20240828040944.GA4005021@coredump.intra.peff.net","subject":"Re: [PATCH] reftable: mark unused parameters in empty iterator functions","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-08-28T08:19:20Z","receivedAt":"2024-08-28T08:19:26Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Aug 28, 2024 at 12:09:44AM -0400, Jeff King wrote:\n> These unused parameters were marked in a68ec8683a (reftable: mark unused\n> parameters in virtual functions, 2024-08-17), but the functions were\n> moved to a new file in a parallel branch via f2406c81b9\n> (reftable/generic: move generic iterator code into iterator interface,\n> 2024-08-22).\n> \n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> This should go on top of ps/reftable-drop-generic. Arguably this could\n> have been done as part of the conflict resolution when merging into next\n> alongside jk/mark-unused-parameters, but at this point I think a\n> separate patch is the best way forward.\n> \n>  reftable/iter.c | 6 +++---\n>  1 file changed, 3 insertions(+), 3 deletions(-)\n> \n> diff --git a/reftable/iter.c b/reftable/iter.c\n> index 97a4642ed5..1d99fe4f7d 100644\n> --- a/reftable/iter.c\n> +++ b/reftable/iter.c\n> @@ -25,17 +25,17 @@ int iterator_next(struct reftable_iterator *it, struct reftable_record *rec)\n>  \treturn it->ops->next(it->iter_arg, rec);\n>  }\n>  \n> -static int empty_iterator_seek(void *arg, struct reftable_record *want)\n> +static int empty_iterator_seek(void *arg UNUSED, struct reftable_record *want UNUSED)\n>  {\n>  \treturn 0;\n>  }\n>  \n> -static int empty_iterator_next(void *arg, struct reftable_record *rec)\n> +static int empty_iterator_next(void *arg UNUSED, struct reftable_record *rec UNUSED)\n>  {\n>  \treturn 1;\n>  }\n>  \n> -static void empty_iterator_close(void *arg)\n> +static void empty_iterator_close(void *arg UNUSED)\n>  {\n>  }\n\nThese changes look obviously correct to me, thanks!\n\nPatrick\n"},{"id":"501784","messageId":"xmqq34mo1si1.fsf@gitster.g","threadId":"62015","inReplyTo":"20240828040944.GA4005021@coredump.intra.peff.net","subject":"Re: [PATCH] reftable: mark unused parameters in empty iterator functions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-28T17:12:22Z","receivedAt":"2024-08-28T17:12:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> This should go on top of ps/reftable-drop-generic. Arguably this could\n> have been done as part of the conflict resolution when merging into next\n> alongside jk/mark-unused-parameters, but at this point I think a\n> separate patch is the best way forward.\n\nAs marking with UNUSED used to be optional before -Wno-unused-param\ngot removed, I agree that it is better to apply this on top to be\nexplicit, rather than burying it in an evil merge with a topic that\nmarked other unrelated parameters as UNUSED.\n"},{"id":"501786","messageId":"20240828181000.GA4034725@coredump.intra.peff.net","threadId":"62015","inReplyTo":"xmqq34mo1si1.fsf@gitster.g","subject":"Re: [PATCH] reftable: mark unused parameters in empty iterator functions","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-08-28T18:10:00Z","receivedAt":"2024-08-28T18:10:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 28, 2024 at 10:12:22AM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > This should go on top of ps/reftable-drop-generic. Arguably this could\n> > have been done as part of the conflict resolution when merging into next\n> > alongside jk/mark-unused-parameters, but at this point I think a\n> > separate patch is the best way forward.\n> \n> As marking with UNUSED used to be optional before -Wno-unused-param\n> got removed, I agree that it is better to apply this on top to be\n> explicit, rather than burying it in an evil merge with a topic that\n> marked other unrelated parameters as UNUSED.\n\n[warning: philosophical rambling ahead]\n\nThe reason I mentioned the merge here is that I think you could argue\nthis is a mis-merge that already happened. Forget for a moment the\nrecent series that makes UNUSED non-optional, and consider the merge of\njk/mark-unused-parameters and ps/reftable-drop-generic.\n\nThe former updated code in reftable/generic.c, and the latter removed\nthat file entirely, so there's a conflict. And it's tempting to say \"ok,\nwe don't care about this code anymore, so take the deletion\", which is\nwhat your merge did. But the code in question was actually moved in that\nseries, via f2406c81b9 (reftable/generic: move generic iterator code\ninto iterator interface, 2024-08-22). So I think the correct resolution\nfor the merge is to move those updates along with the code into the new\nfile; otherwise we are accidentally reverting part of what\njk/mark-unused-parameters did.\n\nThat said, I think you can get pretty philosophical here. Did the\nreftable topic move the code, or did it delete some old code and add\nsome new code that needed the same change? :)\n\nI also think it's pretty hard to notice these kinds of resolutions in\npractice. There's a conflict at the point of deletion, but there's\nnothing in the merge workflow that tells you \"btw, this code is now over\nhere, so you should port the modifications from the side branch over\".\n\nInterestingly, I think a rebase of one topic onto the other might have\nmade it more clear, since the code movement happened in its own step\n(which would make what was going on more obvious). I guess Michael\nHaggerty's \"imerge\" would probably show something similar (actually, I\njust tried it, and it indeed hones in on f2406c81 and 4695c3f3 as the\nsource of the conflict).\n\nAnyway, I am not proposing to do anything different. _If_ we considered\nthe merge of the two topics that is in next to be an incorrect\nresolution, we could repair that when merging to master. But I think\ndoing so is complicated. And certainly the philosophy of \"if something\nis tricky, try to be as explicit as possible\" seems like a good one\nhere.\n\nMostly I just found it kind of an interesting case. :)\n\n-Peff\n"}]}