[PATCH] reftable: mark unused parameters in empty iterator functions

[PATCH] reftable: mark unused parameters in empty iterator functions

[Jeff King]

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)
 {
 }
 
-- 
2.46.0.754.g24c813f009

Re: [PATCH] reftable: mark unused parameters in empty iterator functions

[Patrick Steinhardt]

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

Re: [PATCH] reftable: mark unused parameters in empty iterator functions

[Junio C Hamano]

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.

Re: [PATCH] reftable: mark unused parameters in empty iterator functions

[Jeff King]

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