{"thread":{"id":"59406","subject":"[PATCH 0/4] more unused-parameter fixes","startedAt":"2023-03-17T19:14:17Z","lastAt":"2023-03-17T21:13:51Z","messageCount":8,"participants":["Jeff King","Eric Sunshine","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"473661","messageId":"ZBS77VYL9ZrxKtCN@coredump.intra.peff.net","threadId":"59406","inReplyTo":null,"subject":"[PATCH 0/4] more unused-parameter fixes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-03-17T19:13:49Z","receivedAt":"2023-03-17T19:14:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Here are a few more patches to silence some -Wunused-parameter warnings.\nI've pulled these ones out specifically because they're a little more\ninteresting than just \"slap an UNUSED marker on the variable\".\n\n  [1/4]: mailmap: drop debugging code\n  [2/4]: http: drop unused parameter from start_object_request()\n  [3/4]: http: mark unused parameter in fill_active_slot() callbacks\n  [4/4]: transport: mark unused parameters in fetch_refs_from_bundle()\n\n http-push.c   |  2 +-\n http-walker.c | 11 +++++------\n mailmap.c     | 42 +++---------------------------------------\n transport.c   |  3 ++-\n 4 files changed, 11 insertions(+), 47 deletions(-)\n\n-Peff\n"},{"id":"473663","messageId":"ZBS8illla36MVppa@coredump.intra.peff.net","threadId":"59406","inReplyTo":"ZBS77VYL9ZrxKtCN@coredump.intra.peff.net","subject":"[PATCH 1/4] mailmap: drop debugging code","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-03-17T19:16:26Z","receivedAt":"2023-03-17T19:17:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"There's some debugging code in mailmap.c which is only compiled if you\nmanually tweak the source to set DEBUG_MAILMAP. When it's not set, the\nfallback noop uses static inline functions; we couldn't use macros here\nbecause one of the functions is variadic (and variadic macros were\nforbidden back then, but aren't now). As a result, this triggers\na -Wunused-parameter warning.\n\nWe have a few options here:\n\n  1. Leave it be. Just mark it as UNUSED, or switch to a variadic macro.\n\n  2. Assume the debugging code is useful, compile it always, and trigger\n     it with a run-time flag (e.g., with a trace key). This is pretty\n     easy to do, and carries a pretty small runtime cost.\n\n  3. Assume the debugging is not very useful, and just rip it out. This\n     matches what we did with a similar case in 69c5f17f11 (attr: drop\n     DEBUG_ATTR code, 2022-10-06).\n\nThe debugging flag has been mentioned only three times on the list.\nOnce, when it was added in 2009:\n\n  https://lore.kernel.org/git/cover.1234102794.git.marius@trolltech.com/\n\nIn 2013, when somebody fixed some compilation errors in the conditional\ncode (presumably because they used it while making other changes):\n\n  https://lore.kernel.org/git/1373871253-96480-1-git-send-email-sunshine@sunshineco.com/\n\nAnd finally it seemed to have been useful to somebody in 2021:\n\n  https://lore.kernel.org/git/87eejswql6.fsf@evledraar.gmail.com/\n\nSo it's not totally without value. On the other hand, it's not likely to\nbe useful to non-developers (and certainly isn't if you have to\nrecompile). And using a debugger or adding your own inspection code is\nlikely to be as useful. So I've just dropped the code entirely here.\n\nNote that we do still have to mark a few parameters unused in callback\nfunctions which are passed to string_list_clear_func(). Those get an\nextra pointer with the string being cleared, which we previously fed to\nthe debugging code.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI'm cc-ing folks from those earlier threads. If somebody really wants to\nsalvage it, I can prepare a patch converting it to a trace variable\ninstead, but absent any outcry, I'd go with this approach.\n\n mailmap.c | 42 +++---------------------------------------\n 1 file changed, 3 insertions(+), 39 deletions(-)\n\ndiff --git a/mailmap.c b/mailmap.c\nindex da2589b0822..22e90129ea8 100644\n--- a/mailmap.c\n+++ b/mailmap.c\n@@ -3,16 +3,6 @@\n #include \"mailmap.h\"\n #include \"object-store.h\"\n \n-#define DEBUG_MAILMAP 0\n-#if DEBUG_MAILMAP\n-#define debug_mm(...) fprintf(stderr, __VA_ARGS__)\n-#define debug_str(X) ((X) ? (X) : \"(none)\")\n-#else\n-__attribute__((format (printf, 1, 2)))\n-static inline void debug_mm(const char *format, ...) {}\n-static inline const char *debug_str(const char *s) { return s; }\n-#endif\n-\n const char *git_mailmap_file;\n const char *git_mailmap_blob;\n \n@@ -30,23 +20,17 @@ struct mailmap_entry {\n \tstruct string_list namemap;\n };\n \n-static void free_mailmap_info(void *p, const char *s)\n+static void free_mailmap_info(void *p, const char *s UNUSED)\n {\n \tstruct mailmap_info *mi = (struct mailmap_info *)p;\n-\tdebug_mm(\"mailmap: -- complex: '%s' -> '%s' <%s>\\n\",\n-\t\t s, debug_str(mi->name), debug_str(mi->email));\n \tfree(mi->name);\n \tfree(mi->email);\n \tfree(mi);\n }\n \n-static void free_mailmap_entry(void *p, const char *s)\n+static void free_mailmap_entry(void *p, const char *s UNUSED)\n {\n \tstruct mailmap_entry *me = (struct mailmap_entry *)p;\n-\tdebug_mm(\"mailmap: removing entries for <%s>, with %\"PRIuMAX\" sub-entries\\n\",\n-\t\t s, (uintmax_t)me->namemap.nr);\n-\tdebug_mm(\"mailmap: - simple: '%s' <%s>\\n\",\n-\t\t debug_str(me->name), debug_str(me->email));\n \n \tfree(me->name);\n \tfree(me->email);\n@@ -93,8 +77,6 @@ static void add_mapping(struct string_list *map,\n \t}\n \n \tif (!old_name) {\n-\t\tdebug_mm(\"mailmap: adding (simple) entry for '%s'\\n\", old_email);\n-\n \t\t/* Replace current name and new email for simple entry */\n \t\tif (new_name) {\n \t\t\tfree(me->name);\n@@ -106,15 +88,10 @@ static void add_mapping(struct string_list *map,\n \t\t}\n \t} else {\n \t\tstruct mailmap_info *mi = xcalloc(1, sizeof(struct mailmap_info));\n-\t\tdebug_mm(\"mailmap: adding (complex) entry for '%s'\\n\", old_email);\n \t\tmi->name = xstrdup_or_null(new_name);\n \t\tmi->email = xstrdup_or_null(new_email);\n \t\tstring_list_insert(&me->namemap, old_name)->util = mi;\n \t}\n-\n-\tdebug_mm(\"mailmap:  '%s' <%s> -> '%s' <%s>\\n\",\n-\t\t debug_str(old_name), old_email,\n-\t\t debug_str(new_name), debug_str(new_email));\n }\n \n static char *parse_name_and_email(char *buffer, char **name,\n@@ -250,11 +227,8 @@ int read_mailmap(struct string_list *map)\n \n void clear_mailmap(struct string_list *map)\n {\n-\tdebug_mm(\"mailmap: clearing %\"PRIuMAX\" entries...\\n\",\n-\t\t (uintmax_t)map->nr);\n \tmap->strdup_strings = 1;\n \tstring_list_clear_func(map, free_mailmap_entry);\n-\tdebug_mm(\"mailmap: cleared\\n\");\n }\n \n /*\n@@ -315,10 +289,6 @@ int map_user(struct string_list *map,\n \tstruct string_list_item *item;\n \tstruct mailmap_entry *me;\n \n-\tdebug_mm(\"map_user: map '%.*s' <%.*s>\\n\",\n-\t\t (int)*namelen, debug_str(*name),\n-\t\t (int)*emaillen, debug_str(*email));\n-\n \titem = lookup_prefix(map, *email, *emaillen);\n \tif (item) {\n \t\tme = (struct mailmap_entry *)item->util;\n@@ -336,10 +306,8 @@ int map_user(struct string_list *map,\n \t}\n \tif (item) {\n \t\tstruct mailmap_info *mi = (struct mailmap_info *)item->util;\n-\t\tif (mi->name == NULL && mi->email == NULL) {\n-\t\t\tdebug_mm(\"map_user:  -- (no simple mapping)\\n\");\n+\t\tif (mi->name == NULL && mi->email == NULL)\n \t\t\treturn 0;\n-\t\t}\n \t\tif (mi->email) {\n \t\t\t\t*email = mi->email;\n \t\t\t\t*emaillen = strlen(*email);\n@@ -348,11 +316,7 @@ int map_user(struct string_list *map,\n \t\t\t\t*name = mi->name;\n \t\t\t\t*namelen = strlen(*name);\n \t\t}\n-\t\tdebug_mm(\"map_user:  to '%.*s' <%.*s>\\n\",\n-\t\t\t (int)*namelen, debug_str(*name),\n-\t\t\t (int)*emaillen, debug_str(*email));\n \t\treturn 1;\n \t}\n-\tdebug_mm(\"map_user:  --\\n\");\n \treturn 0;\n }\n-- \n2.40.0.581.g8d688c70eca\n\n"},{"id":"473662","messageId":"ZBS8kqjaq2DIZMEG@coredump.intra.peff.net","threadId":"59406","inReplyTo":"ZBS77VYL9ZrxKtCN@coredump.intra.peff.net","subject":"[PATCH 2/4] http: drop unused parameter from start_object_request()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-03-17T19:16:34Z","receivedAt":"2023-03-17T19:17:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We take a \"walker\" parameter for the request, but don't actually look at\nit. This is due to 5424bc557f (http*: add helper methods for fetching\nobjects (loose), 2009-06-06). Before then, we consulted the \"walker\"\nstruct to tell us if we should be verbose, but now those messages are\nprinted elsewhere.\n\nLet's drop the unused parameter to make -Wunused-parameter happy.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n http-walker.c | 7 +++----\n 1 file changed, 3 insertions(+), 4 deletions(-)\n\ndiff --git a/http-walker.c b/http-walker.c\nindex b8f0f98ae14..6b9bdb529b5 100644\n--- a/http-walker.c\n+++ b/http-walker.c\n@@ -52,8 +52,7 @@ static void fetch_alternates(struct walker *walker, const char *base);\n \n static void process_object_response(void *callback_data);\n \n-static void start_object_request(struct walker *walker,\n-\t\t\t\t struct object_request *obj_req)\n+static void start_object_request(struct object_request *obj_req)\n {\n \tstruct active_request_slot *slot;\n \tstruct http_object_request *req;\n@@ -110,7 +109,7 @@ static void process_object_response(void *callback_data)\n \t\t\tobj_req->repo =\n \t\t\t\tobj_req->repo->next;\n \t\t\trelease_http_object_request(obj_req->req);\n-\t\t\tstart_object_request(walker, obj_req);\n+\t\t\tstart_object_request(obj_req);\n \t\t\treturn;\n \t\t}\n \t}\n@@ -138,7 +137,7 @@ static int fill_active_slot(struct walker *walker)\n \t\t\tif (has_object_file(&obj_req->oid))\n \t\t\t\tobj_req->state = COMPLETE;\n \t\t\telse {\n-\t\t\t\tstart_object_request(walker, obj_req);\n+\t\t\t\tstart_object_request(obj_req);\n \t\t\t\treturn 1;\n \t\t\t}\n \t\t}\n-- \n2.40.0.581.g8d688c70eca\n\n"},{"id":"473664","messageId":"ZBS8tD4KjwQ8aYov@coredump.intra.peff.net","threadId":"59406","inReplyTo":"ZBS77VYL9ZrxKtCN@coredump.intra.peff.net","subject":"[PATCH 3/4] http: mark unused parameter in fill_active_slot() callbacks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-03-17T19:17:08Z","receivedAt":"2023-03-17T19:17:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We have a generic \"fill\" function that is used by both the dumb http\npush and fetch code paths. It takes a void parameter in case the caller\nwants to pass along extra data, but (since the previous commit) neither\ndoes so.\n\nSo we could simply drop the extra parameter. But since it's good\npractice to provide a void pointer for in callback functions, we'll\nleave it here for the future, and just annotate it as unused (to appease\n-Wunused-parameter).\n\nWhile we're marking it, let's also fix the type in http-walker's\nfunction to have the correct \"void\" type. The original had to cast the\nfunction pointer and was technically undefined behavior (though\ngenerally OK in practice).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nEvery time I touch these files I dream of just deleting all of the\ndumb-http completely.\n\n http-push.c   | 2 +-\n http-walker.c | 4 ++--\n 2 files changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/http-push.c b/http-push.c\nindex 7f71316456c..8eaba6ddf0a 100644\n--- a/http-push.c\n+++ b/http-push.c\n@@ -601,7 +601,7 @@ static void finish_request(struct transfer_request *request)\n }\n \n static int is_running_queue;\n-static int fill_active_slot(void *unused)\n+static int fill_active_slot(void *data UNUSED)\n {\n \tstruct transfer_request *request;\n \ndiff --git a/http-walker.c b/http-walker.c\nindex 6b9bdb529b5..32369aa2d74 100644\n--- a/http-walker.c\n+++ b/http-walker.c\n@@ -126,7 +126,7 @@ static void release_object_request(struct object_request *obj_req)\n \tfree(obj_req);\n }\n \n-static int fill_active_slot(struct walker *walker)\n+static int fill_active_slot(void *data UNUSED)\n {\n \tstruct object_request *obj_req;\n \tstruct list_head *pos, *tmp, *head = &object_queue_head;\n@@ -612,7 +612,7 @@ struct walker *get_http_walker(const char *url)\n \twalker->cleanup = cleanup;\n \twalker->data = data;\n \n-\tadd_fill_function(walker, (int (*)(void *)) fill_active_slot);\n+\tadd_fill_function(NULL, fill_active_slot);\n \n \treturn walker;\n }\n-- \n2.40.0.581.g8d688c70eca\n\n"},{"id":"473665","messageId":"ZBS8vt97YJj3wMxn@coredump.intra.peff.net","threadId":"59406","inReplyTo":"ZBS77VYL9ZrxKtCN@coredump.intra.peff.net","subject":"[PATCH 4/4] transport: mark unused parameters in fetch_refs_from_bundle()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-03-17T19:17:18Z","receivedAt":"2023-03-17T19:17:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We don't look at the \"to_fetch\" or \"nr_heads\" parameters at all. At\nfirst glance this seems like a bug (or at least pessimisation), because\nit means we fetch more objects from the bundle than we actually need.\nBut the bundle does not have any way of computing the set of reachable\nobjects itself (we'd have to pull all of the objects out to walk them).\nAnd anyway, we've probably already paid most of the cost of grabbing the\nobjects, since we must copy the bundle locally before accessing it.\n\nSo it's perfectly reasonable for the bundle code to just pull everything\ninto the local object store. Unneeded objects can be dropped later via\ngc, etc.\n\nBut we should mark these unused parameters as such to avoid the wrath of\n-Wunused-parameter.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n transport.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/transport.c b/transport.c\nindex 81950bf755f..c5abf26f31e 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -167,7 +167,8 @@ static struct ref *get_refs_from_bundle(struct transport *transport,\n }\n \n static int fetch_refs_from_bundle(struct transport *transport,\n-\t\t\t       int nr_heads, struct ref **to_fetch)\n+\t\t\t\t  int nr_heads UNUSED,\n+\t\t\t\t  struct ref **to_fetch UNUSED)\n {\n \tstruct bundle_transport_data *data = transport->data;\n \tstruct strvec extra_index_pack_args = STRVEC_INIT;\n-- \n2.40.0.581.g8d688c70eca\n"},{"id":"473668","messageId":"CAPig+cQ9MNjEFHA07CM+c+qfTzNm99D=npLPD9qY079kkCzH6g@mail.gmail.com","threadId":"59406","inReplyTo":"ZBS8illla36MVppa@coredump.intra.peff.net","subject":"Re: [PATCH 1/4] mailmap: drop debugging code","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2023-03-17T20:08:43Z","receivedAt":"2023-03-17T20:08:58Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Mar 17, 2023 at 3:16 PM Jeff King <peff@peff.net> wrote:\n> There's some debugging code in mailmap.c which is only compiled if you\n> manually tweak the source to set DEBUG_MAILMAP. When it's not set, the\n> fallback noop uses static inline functions; we couldn't use macros here\n> because one of the functions is variadic (and variadic macros were\n> forbidden back then, but aren't now). As a result, this triggers\n> a -Wunused-parameter warning.\n>\n> We have a few options here:\n>\n>   1. Leave it be. Just mark it as UNUSED, or switch to a variadic macro.\n>\n>   2. Assume the debugging code is useful, compile it always, and trigger\n>      it with a run-time flag (e.g., with a trace key). This is pretty\n>      easy to do, and carries a pretty small runtime cost.\n>\n>   3. Assume the debugging is not very useful, and just rip it out. This\n>      matches what we did with a similar case in 69c5f17f11 (attr: drop\n>      DEBUG_ATTR code, 2022-10-06).\n>\n> The debugging flag has been mentioned only three times on the list.\n> Once, when it was added in 2009:\n>\n>   https://lore.kernel.org/git/cover.1234102794.git.marius@trolltech.com/\n>\n> In 2013, when somebody fixed some compilation errors in the conditional\n> code (presumably because they used it while making other changes):\n>\n>   https://lore.kernel.org/git/1373871253-96480-1-git-send-email-sunshine@sunshineco.com/\n>\n> And finally it seemed to have been useful to somebody in 2021:\n>\n>   https://lore.kernel.org/git/87eejswql6.fsf@evledraar.gmail.com/\n\nNit: s/2021/2020/\n\n> So it's not totally without value. On the other hand, it's not likely to\n> be useful to non-developers (and certainly isn't if you have to\n> recompile). And using a debugger or adding your own inspection code is\n> likely to be as useful. So I've just dropped the code entirely here.\n>\n> Note that we do still have to mark a few parameters unused in callback\n> functions which are passed to string_list_clear_func(). Those get an\n> extra pointer with the string being cleared, which we previously fed to\n> the debugging code.\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> I'm cc-ing folks from those earlier threads. If somebody really wants to\n> salvage it, I can prepare a patch converting it to a trace variable\n> instead, but absent any outcry, I'd go with this approach.\n\nAs one of the mentioned anonymous \"sombody\"s, I have no objection.\n"},{"id":"473670","messageId":"xmqq7cvfgm0v.fsf@gitster.g","threadId":"59406","inReplyTo":"ZBS8vt97YJj3wMxn@coredump.intra.peff.net","subject":"Re: [PATCH 4/4] transport: mark unused parameters in fetch_refs_from_bundle()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-17T20:28:48Z","receivedAt":"2023-03-17T20:29:29Z","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> We don't look at the \"to_fetch\" or \"nr_heads\" parameters at all. At\n> first glance this seems like a bug (or at least pessimisation), because\n> it means we fetch more objects from the bundle than we actually need.\n> But the bundle does not have any way of computing the set of reachable\n> objects itself (we'd have to pull all of the objects out to walk them).\n> And anyway, we've probably already paid most of the cost of grabbing the\n> objects, since we must copy the bundle locally before accessing it.\n>\n> So it's perfectly reasonable for the bundle code to just pull everything\n> into the local object store. Unneeded objects can be dropped later via\n> gc, etc.\n>\n> But we should mark these unused parameters as such to avoid the wrath of\n> -Wunused-parameter.\n\nVery well described, and I agree with the reasoning.\n\nWill queue all four.  Thanks.\n\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  transport.c | 3 ++-\n>  1 file changed, 2 insertions(+), 1 deletion(-)\n>\n> diff --git a/transport.c b/transport.c\n> index 81950bf755f..c5abf26f31e 100644\n> --- a/transport.c\n> +++ b/transport.c\n> @@ -167,7 +167,8 @@ static struct ref *get_refs_from_bundle(struct transport *transport,\n>  }\n>  \n>  static int fetch_refs_from_bundle(struct transport *transport,\n> -\t\t\t       int nr_heads, struct ref **to_fetch)\n> +\t\t\t\t  int nr_heads UNUSED,\n> +\t\t\t\t  struct ref **to_fetch UNUSED)\n>  {\n>  \tstruct bundle_transport_data *data = transport->data;\n>  \tstruct strvec extra_index_pack_args = STRVEC_INIT;\n"},{"id":"473673","messageId":"ZBTYABsQUOCZ2mMe@coredump.intra.peff.net","threadId":"59406","inReplyTo":"CAPig+cQ9MNjEFHA07CM+c+qfTzNm99D=npLPD9qY079kkCzH6g@mail.gmail.com","subject":"Re: [PATCH 1/4] mailmap: drop debugging code","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-03-17T21:13:36Z","receivedAt":"2023-03-17T21:13:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 17, 2023 at 04:08:43PM -0400, Eric Sunshine wrote:\n\n> > In 2013, when somebody fixed some compilation errors in the conditional\n> > code (presumably because they used it while making other changes):\n> >\n> >   https://lore.kernel.org/git/1373871253-96480-1-git-send-email-sunshine@sunshineco.com/\n> >\n> > And finally it seemed to have been useful to somebody in 2021:\n> >\n> >   https://lore.kernel.org/git/87eejswql6.fsf@evledraar.gmail.com/\n> \n> Nit: s/2021/2020/\n\nOops. I'm not sure how I bungled that. :)\n\n> > I'm cc-ing folks from those earlier threads. If somebody really wants to\n> > salvage it, I can prepare a patch converting it to a trace variable\n> > instead, but absent any outcry, I'd go with this approach.\n> \n> As one of the mentioned anonymous \"sombody\"s, I have no objection.\n\nThanks!\n\n-Peff\n"}]}