threads / patch / 59406

patch, 4 partsmore unused-parameter fixes

Subject: [PATCH 0/4] more unused-parameter fixes

## tl;dr

8 messages between Mar 17, 2023 and Mar 17, 2023. Diffs are folded; open one to read it.

replies: 7people: 3as markdown or json

Jeff King· Mar 17, 2023, 19:13 UTC · lore

Here are a few more patches to silence some -Wunused-parameter warnings. I've pulled these ones out specifically because they're a little more interesting than just "slap an UNUSED marker on the variable".

  [1/4]: mailmap: drop debugging code
  [2/4]: http: drop unused parameter from start_object_request()
  [3/4]: http: mark unused parameter in fill_active_slot() callbacks
  [4/4]: transport: mark unused parameters in fetch_refs_from_bundle()
 http-push.c   |  2 +-
 http-walker.c | 11 +++++------
 mailmap.c     | 42 +++---------------------------------------
 transport.c   |  3 ++-
 4 files changed, 11 insertions(+), 47 deletions(-)
-Peff
Jeff King· Mar 17, 2023, 19:16 UTC · re: Jeff King · lore

[PATCH 1/4] mailmap: drop debugging code

There's some debugging code in mailmap.c which is only compiled if you manually tweak the source to set DEBUG_MAILMAP. When it's not set, the fallback noop uses static inline functions; we couldn't use macros here because one of the functions is variadic (and variadic macros were forbidden back then, but aren't now). As a result, this triggers a -Wunused-parameter warning.

We have a few options here:
  1. Leave it be. Just mark it as UNUSED, or switch to a variadic macro.
  2. Assume the debugging code is useful, compile it always, and trigger
     it with a run-time flag (e.g., with a trace key). This is pretty
     easy to do, and carries a pretty small runtime cost.
  3. Assume the debugging is not very useful, and just rip it out. This
     matches what we did with a similar case in 69c5f17f11 (attr: drop
     DEBUG_ATTR code, 2022-10-06).

The debugging flag has been mentioned only three times on the list. Once, when it was added in 2009:

  https://lore.kernel.org/git/cover.1234102794.git.marius@trolltech.com/

In 2013, when somebody fixed some compilation errors in the conditional code (presumably because they used it while making other changes):

  https://lore.kernel.org/git/1373871253-96480-1-git-send-email-sunshine@sunshineco.com/
And finally it seemed to have been useful to somebody in 2021:
  https://lore.kernel.org/git/87eejswql6.fsf@evledraar.gmail.com/

So it's not totally without value. On the other hand, it's not likely to be useful to non-developers (and certainly isn't if you have to recompile). And using a debugger or adding your own inspection code is likely to be as useful. So I've just dropped the code entirely here.

Note that we do still have to mark a few parameters unused in callback functions which are passed to string_list_clear_func(). Those get an extra pointer with the string being cleared, which we previously fed to the debugging code.

Signed-off-by: Jeff King <peff@peff.net>
---
I'm cc-ing folks from those earlier threads. If somebody really wants to
salvage it, I can prepare a patch converting it to a trace variable
instead, but absent any outcry, I'd go with this approach.
 mailmap.c | 42 +++---------------------------------------
 1 file changed, 3 insertions(+), 39 deletions(-)
Show changes to mailmap.c +3 −39
diff --git a/mailmap.c b/mailmap.c
index da2589b0822..22e90129ea8 100644
--- a/mailmap.c
+++ b/mailmap.c
@@ -3,16 +3,6 @@
 #include "mailmap.h"
 #include "object-store.h"
 
-#define DEBUG_MAILMAP 0
-#if DEBUG_MAILMAP
-#define debug_mm(...) fprintf(stderr, __VA_ARGS__)
-#define debug_str(X) ((X) ? (X) : "(none)")
-#else
-__attribute__((format (printf, 1, 2)))
-static inline void debug_mm(const char *format, ...) {}
-static inline const char *debug_str(const char *s) { return s; }
-#endif
-
 const char *git_mailmap_file;
 const char *git_mailmap_blob;
 
@@ -30,23 +20,17 @@ struct mailmap_entry {
 	struct string_list namemap;
 };
 
-static void free_mailmap_info(void *p, const char *s)
+static void free_mailmap_info(void *p, const char *s UNUSED)
 {
 	struct mailmap_info *mi = (struct mailmap_info *)p;
-	debug_mm("mailmap: -- complex: '%s' -> '%s' <%s>\n",
-		 s, debug_str(mi->name), debug_str(mi->email));
 	free(mi->name);
 	free(mi->email);
 	free(mi);
 }
 
-static void free_mailmap_entry(void *p, const char *s)
+static void free_mailmap_entry(void *p, const char *s UNUSED)
 {
 	struct mailmap_entry *me = (struct mailmap_entry *)p;
-	debug_mm("mailmap: removing entries for <%s>, with %"PRIuMAX" sub-entries\n",
-		 s, (uintmax_t)me->namemap.nr);
-	debug_mm("mailmap: - simple: '%s' <%s>\n",
-		 debug_str(me->name), debug_str(me->email));
 
 	free(me->name);
 	free(me->email);
@@ -93,8 +77,6 @@ static void add_mapping(struct string_list *map,
 	}
 
 	if (!old_name) {
-		debug_mm("mailmap: adding (simple) entry for '%s'\n", old_email);
-
 		/* Replace current name and new email for simple entry */
 		if (new_name) {
 			free(me->name);
@@ -106,15 +88,10 @@ static void add_mapping(struct string_list *map,
 		}
 	} else {
 		struct mailmap_info *mi = xcalloc(1, sizeof(struct mailmap_info));
-		debug_mm("mailmap: adding (complex) entry for '%s'\n", old_email);
 		mi->name = xstrdup_or_null(new_name);
 		mi->email = xstrdup_or_null(new_email);
 		string_list_insert(&me->namemap, old_name)->util = mi;
 	}
-
-	debug_mm("mailmap:  '%s' <%s> -> '%s' <%s>\n",
-		 debug_str(old_name), old_email,
-		 debug_str(new_name), debug_str(new_email));
 }
 
 static char *parse_name_and_email(char *buffer, char **name,
@@ -250,11 +227,8 @@ int read_mailmap(struct string_list *map)
 
 void clear_mailmap(struct string_list *map)
 {
-	debug_mm("mailmap: clearing %"PRIuMAX" entries...\n",
-		 (uintmax_t)map->nr);
 	map->strdup_strings = 1;
 	string_list_clear_func(map, free_mailmap_entry);
-	debug_mm("mailmap: cleared\n");
 }
 
 /*
@@ -315,10 +289,6 @@ int map_user(struct string_list *map,
 	struct string_list_item *item;
 	struct mailmap_entry *me;
 
-	debug_mm("map_user: map '%.*s' <%.*s>\n",
-		 (int)*namelen, debug_str(*name),
-		 (int)*emaillen, debug_str(*email));
-
 	item = lookup_prefix(map, *email, *emaillen);
 	if (item) {
 		me = (struct mailmap_entry *)item->util;
@@ -336,10 +306,8 @@ int map_user(struct string_list *map,
 	}
 	if (item) {
 		struct mailmap_info *mi = (struct mailmap_info *)item->util;
-		if (mi->name == NULL && mi->email == NULL) {
-			debug_mm("map_user:  -- (no simple mapping)\n");
+		if (mi->name == NULL && mi->email == NULL)
 			return 0;
-		}
 		if (mi->email) {
 				*email = mi->email;
 				*emaillen = strlen(*email);
@@ -348,11 +316,7 @@ int map_user(struct string_list *map,
 				*name = mi->name;
 				*namelen = strlen(*name);
 		}
-		debug_mm("map_user:  to '%.*s' <%.*s>\n",
-			 (int)*namelen, debug_str(*name),
-			 (int)*emaillen, debug_str(*email));
 		return 1;
 	}
-	debug_mm("map_user:  --\n");
 	return 0;
 }
-- 
2.40.0.581.g8d688c70eca
Eric Sunshine· Mar 17, 2023, 20:08 UTC · re: Jeff King · lore

Re: [PATCH 1/4] mailmap: drop debugging code

On Fri, Mar 17, 2023 at 3:16 PM Jeff King <peff@peff.net> wrote:
Show 32 quoted lines
> There's some debugging code in mailmap.c which is only compiled if you
> manually tweak the source to set DEBUG_MAILMAP. When it's not set, the
> fallback noop uses static inline functions; we couldn't use macros here
> because one of the functions is variadic (and variadic macros were
> forbidden back then, but aren't now). As a result, this triggers
> a -Wunused-parameter warning.
>
> We have a few options here:
>
>   1. Leave it be. Just mark it as UNUSED, or switch to a variadic macro.
>
>   2. Assume the debugging code is useful, compile it always, and trigger
>      it with a run-time flag (e.g., with a trace key). This is pretty
>      easy to do, and carries a pretty small runtime cost.
>
>   3. Assume the debugging is not very useful, and just rip it out. This
>      matches what we did with a similar case in 69c5f17f11 (attr: drop
>      DEBUG_ATTR code, 2022-10-06).
>
> The debugging flag has been mentioned only three times on the list.
> Once, when it was added in 2009:
>
>   https://lore.kernel.org/git/cover.1234102794.git.marius@trolltech.com/
>
> In 2013, when somebody fixed some compilation errors in the conditional
> code (presumably because they used it while making other changes):
>
>   https://lore.kernel.org/git/1373871253-96480-1-git-send-email-sunshine@sunshineco.com/
>
> And finally it seemed to have been useful to somebody in 2021:
>
>   https://lore.kernel.org/git/87eejswql6.fsf@evledraar.gmail.com/
Nit: s/2021/2020/
Show 15 quoted lines
> So it's not totally without value. On the other hand, it's not likely to
> be useful to non-developers (and certainly isn't if you have to
> recompile). And using a debugger or adding your own inspection code is
> likely to be as useful. So I've just dropped the code entirely here.
>
> Note that we do still have to mark a few parameters unused in callback
> functions which are passed to string_list_clear_func(). Those get an
> extra pointer with the string being cleared, which we previously fed to
> the debugging code.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> I'm cc-ing folks from those earlier threads. If somebody really wants to
> salvage it, I can prepare a patch converting it to a trace variable
> instead, but absent any outcry, I'd go with this approach.
As one of the mentioned anonymous "sombody"s, I have no objection.
Jeff King· Mar 17, 2023, 21:13 UTC · re: Eric Sunshine · lore

Re: [PATCH 1/4] mailmap: drop debugging code

On Fri, Mar 17, 2023 at 04:08:43PM -0400, Eric Sunshine wrote:
Show 10 quoted lines
> > In 2013, when somebody fixed some compilation errors in the conditional
> > code (presumably because they used it while making other changes):
> >
> >   https://lore.kernel.org/git/1373871253-96480-1-git-send-email-sunshine@sunshineco.com/
> >
> > And finally it seemed to have been useful to somebody in 2021:
> >
> >   https://lore.kernel.org/git/87eejswql6.fsf@evledraar.gmail.com/
> 
> Nit: s/2021/2020/
Oops. I'm not sure how I bungled that. :)
Show 5 quoted lines
> > I'm cc-ing folks from those earlier threads. If somebody really wants to
> > salvage it, I can prepare a patch converting it to a trace variable
> > instead, but absent any outcry, I'd go with this approach.
> 
> As one of the mentioned anonymous "sombody"s, I have no objection.
Thanks!
-Peff
Jeff King· Mar 17, 2023, 19:16 UTC · re: Jeff King · lore

[PATCH 2/4] http: drop unused parameter from start_object_request()

We take a "walker" parameter for the request, but don't actually look at it. This is due to 5424bc557f (http*: add helper methods for fetching objects (loose), 2009-06-06). Before then, we consulted the "walker" struct to tell us if we should be verbose, but now those messages are printed elsewhere.

Let's drop the unused parameter to make -Wunused-parameter happy.
Signed-off-by: Jeff King <peff@peff.net>
---
 http-walker.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)
Show changes to http-walker.c +3 −4
diff --git a/http-walker.c b/http-walker.c
index b8f0f98ae14..6b9bdb529b5 100644
--- a/http-walker.c
+++ b/http-walker.c
@@ -52,8 +52,7 @@ static void fetch_alternates(struct walker *walker, const char *base);
 
 static void process_object_response(void *callback_data);
 
-static void start_object_request(struct walker *walker,
-				 struct object_request *obj_req)
+static void start_object_request(struct object_request *obj_req)
 {
 	struct active_request_slot *slot;
 	struct http_object_request *req;
@@ -110,7 +109,7 @@ static void process_object_response(void *callback_data)
 			obj_req->repo =
 				obj_req->repo->next;
 			release_http_object_request(obj_req->req);
-			start_object_request(walker, obj_req);
+			start_object_request(obj_req);
 			return;
 		}
 	}
@@ -138,7 +137,7 @@ static int fill_active_slot(struct walker *walker)
 			if (has_object_file(&obj_req->oid))
 				obj_req->state = COMPLETE;
 			else {
-				start_object_request(walker, obj_req);
+				start_object_request(obj_req);
 				return 1;
 			}
 		}
-- 
2.40.0.581.g8d688c70eca
Jeff King· Mar 17, 2023, 19:17 UTC · re: Jeff King · lore

[PATCH 3/4] http: mark unused parameter in fill_active_slot() callbacks

We have a generic "fill" function that is used by both the dumb http push and fetch code paths. It takes a void parameter in case the caller wants to pass along extra data, but (since the previous commit) neither does so.

So we could simply drop the extra parameter. But since it's good practice to provide a void pointer for in callback functions, we'll leave it here for the future, and just annotate it as unused (to appease -Wunused-parameter).

While we're marking it, let's also fix the type in http-walker's function to have the correct "void" type. The original had to cast the function pointer and was technically undefined behavior (though generally OK in practice).

Signed-off-by: Jeff King <peff@peff.net>
---
Every time I touch these files I dream of just deleting all of the
dumb-http completely.
 http-push.c   | 2 +-
 http-walker.c | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)
Show changes to 2 files +3 −3

http-push.c, http-walker.c

diff --git a/http-push.c b/http-push.c
index 7f71316456c..8eaba6ddf0a 100644
--- a/http-push.c
+++ b/http-push.c
@@ -601,7 +601,7 @@ static void finish_request(struct transfer_request *request)
 }
 
 static int is_running_queue;
-static int fill_active_slot(void *unused)
+static int fill_active_slot(void *data UNUSED)
 {
 	struct transfer_request *request;
 
diff --git a/http-walker.c b/http-walker.c
index 6b9bdb529b5..32369aa2d74 100644
--- a/http-walker.c
+++ b/http-walker.c
@@ -126,7 +126,7 @@ static void release_object_request(struct object_request *obj_req)
 	free(obj_req);
 }
 
-static int fill_active_slot(struct walker *walker)
+static int fill_active_slot(void *data UNUSED)
 {
 	struct object_request *obj_req;
 	struct list_head *pos, *tmp, *head = &object_queue_head;
@@ -612,7 +612,7 @@ struct walker *get_http_walker(const char *url)
 	walker->cleanup = cleanup;
 	walker->data = data;
 
-	add_fill_function(walker, (int (*)(void *)) fill_active_slot);
+	add_fill_function(NULL, fill_active_slot);
 
 	return walker;
 }
-- 
2.40.0.581.g8d688c70eca
Jeff King· Mar 17, 2023, 19:17 UTC · re: Jeff King · lore

[PATCH 4/4] transport: mark unused parameters in fetch_refs_from_bundle()

We don't look at the "to_fetch" or "nr_heads" parameters at all. At first glance this seems like a bug (or at least pessimisation), because it means we fetch more objects from the bundle than we actually need. But the bundle does not have any way of computing the set of reachable objects itself (we'd have to pull all of the objects out to walk them). And anyway, we've probably already paid most of the cost of grabbing the objects, since we must copy the bundle locally before accessing it.

So it's perfectly reasonable for the bundle code to just pull everything into the local object store. Unneeded objects can be dropped later via gc, etc.

But we should mark these unused parameters as such to avoid the wrath of -Wunused-parameter.

Signed-off-by: Jeff King <peff@peff.net>
---
 transport.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
Show changes to transport.c +2 −1
diff --git a/transport.c b/transport.c
index 81950bf755f..c5abf26f31e 100644
--- a/transport.c
+++ b/transport.c
@@ -167,7 +167,8 @@ static struct ref *get_refs_from_bundle(struct transport *transport,
 }
 
 static int fetch_refs_from_bundle(struct transport *transport,
-			       int nr_heads, struct ref **to_fetch)
+				  int nr_heads UNUSED,
+				  struct ref **to_fetch UNUSED)
 {
 	struct bundle_transport_data *data = transport->data;
 	struct strvec extra_index_pack_args = STRVEC_INIT;
-- 
2.40.0.581.g8d688c70eca
Junio C Hamano· Mar 17, 2023, 20:28 UTC · re: Jeff King · lore

Re: [PATCH 4/4] transport: mark unused parameters in fetch_refs_from_bundle()

Jeff King <peff@peff.net> writes:
Show 14 quoted lines
> We don't look at the "to_fetch" or "nr_heads" parameters at all. At
> first glance this seems like a bug (or at least pessimisation), because
> it means we fetch more objects from the bundle than we actually need.
> But the bundle does not have any way of computing the set of reachable
> objects itself (we'd have to pull all of the objects out to walk them).
> And anyway, we've probably already paid most of the cost of grabbing the
> objects, since we must copy the bundle locally before accessing it.
>
> So it's perfectly reasonable for the bundle code to just pull everything
> into the local object store. Unneeded objects can be dropped later via
> gc, etc.
>
> But we should mark these unused parameters as such to avoid the wrath of
> -Wunused-parameter.
Very well described, and I agree with the reasoning.
Will queue all four.  Thanks.
Show 20 quoted lines
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  transport.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/transport.c b/transport.c
> index 81950bf755f..c5abf26f31e 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -167,7 +167,8 @@ static struct ref *get_refs_from_bundle(struct transport *transport,
>  }
>  
>  static int fetch_refs_from_bundle(struct transport *transport,
> -			       int nr_heads, struct ref **to_fetch)
> +				  int nr_heads UNUSED,
> +				  struct ref **to_fetch UNUSED)
>  {
>  	struct bundle_transport_data *data = transport->data;
>  	struct strvec extra_index_pack_args = STRVEC_INIT;

← back to recent threads