{"thread":{"id":"62020","subject":"[PATCH 0/2] clean up some MAYBE_UNUSED cases","startedAt":"2024-08-29T20:08:08Z","lastAt":"2024-08-30T16:30:17Z","messageCount":7,"participants":["Jeff King","Eric Sunshine","Junio C Hamano","Patrick Steinhardt"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"501865","messageId":"20240829200807.GA430283@coredump.intra.peff.net","threadId":"62020","inReplyTo":null,"subject":"[PATCH 0/2] clean up some MAYBE_UNUSED cases","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-08-29T20:08:07Z","receivedAt":"2024-08-29T20:08:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The discussion about MAYBE_UNUSED over in[1] made me wonder how it was\nused in practice. This series cleans up a few spots where I think it is\nbeing misused.\n\n[1] https://lore.kernel.org/git/xmqqseunxtks.fsf_-_@gitster.g/\n\nThe remaining grep hits are:\n\n - fsmonitor has a few functions with sometimes-unused parameters based\n on conditional compilation (try_to_run_foreground_daemon() and\n check_for_incompatible()). This is a good use of MAYBE_UNUSED.\n\n - builtin/gc.c's check_crontab_process(). The whole function is marked\n as MAYBE_UNUSED here, which is a little funny. It's because\n is_crontab_available() may or may not call us based on __APPLE__.\n Should we conditionally define the function, too, in that case? It\n would mean repeating the #ifdef. Alternatively, we could define it\n like this:\n\n #ifdef __APPLE__\n static int check_crontab_process(const char *cmd UNUSED)\n {\n return 0;\n }\n #else\n static int check_crontab_process(const char *cmd UNUSED)\n {\n [...the real function...]\n }\n #endif\n\n But I think we're getting into \"well, this is how I would have\n written it\" territory, and it doesn't matter much either way in\n practice. It's probably better to just leave it alone.\n\n - commit-slab marks auto-generated static functions with MAYBE_UNUSED,\n since it doesn't know which ones actually need to be instantiated.\n This was the original thing we added MAYBE_UNUSED for.\n\n - khash does something similar when auto-generating functions.\n\n - test_bitmap_commits() marks a local variable as MAYBE_UNUSED! In\n fact it's completely unused by the function itself, but the macro\n expansion of kh_foreach() assigns to it. So we need the variable to\n exist to pass into the macro, but the compiler warning is triggered\n on the expanded code. We have kh_foreach_value(), but not\n kh_foreach_key(), which is what we'd want here. It wouldn't be hard\n to add it, but the MAYBE_UNUSED here is doing a fine job of\n suppressing the warning (and presumably an optimizing compiler\n removes the useless assignment).\n\nI prepared this on top of what's queued in jk/unused-parameters (which\nhelps with making sure the annotations are all correct), but it could be\napplied separately.\n\n [1/2]: gc: drop MAYBE_UNUSED annotation from used parameter\n [2/2]: grep: prefer UNUSED to MAYBE_UNUSED for pcre allocators\n\n builtin/gc.c | 2 +-\n grep.c | 4 ++--\n 2 files changed, 3 insertions(+), 3 deletions(-)\n\n-Peff\n"},{"id":"501866","messageId":"20240829200833.GA432235@coredump.intra.peff.net","threadId":"62020","inReplyTo":"20240829200807.GA430283@coredump.intra.peff.net","subject":"[PATCH 1/2] gc: drop MAYBE_UNUSED annotation from used parameter","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-08-29T20:08:33Z","receivedAt":"2024-08-29T20:08:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The \"opts\" parameter is always used, so marking it with MAYBE_UNUSED is\njust confusing.\n\nThis annotation goes back to 41abfe15d9 (maintenance: add pack-refs\ntask, 2021-02-09), when it really was unused. Back then we did not have\nthe UNUSED macro that would complain if the code changed to use the\nparameter. So when we started using it in bfc2f9eb8e (builtin/gc:\nforward git-gc(1)'s `--auto` flag when packing refs, 2024-03-25), nobody\nnoticed.\n\nSigned-off-by: Jeff King \n---\n builtin/gc.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 0e361253e3..7dac971405 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -260,7 +260,7 @@ static int pack_refs_condition(UNUSED struct gc_config *cfg)\n \treturn 1;\n }\n \n-static int maintenance_task_pack_refs(MAYBE_UNUSED struct maintenance_run_opts *opts,\n+static int maintenance_task_pack_refs(struct maintenance_run_opts *opts,\n \t\t\t\t UNUSED struct gc_config *cfg)\n {\n \tstruct child_process cmd = CHILD_PROCESS_INIT;\n-- \n2.46.0.761.g18aface1ae\n\n"},{"id":"501867","messageId":"20240829200953.GB432235@coredump.intra.peff.net","threadId":"62020","inReplyTo":"20240829200807.GA430283@coredump.intra.peff.net","subject":"[PATCH 2/2] grep: prefer UNUSED to MAYBE_UNUSED for pcre allocators","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-08-29T20:09:53Z","receivedAt":"2024-08-29T20:09:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We prove custom malloc/free callbacks for the pcre library to use. Those\ntake an extra \"data\" parameter, but we don't use it. Back when these\nwere added in 513f2b0bbd (grep: make PCRE2 aware of custom allocator,\n2019-10-16), we only had MAYBE_UNUSED. But these days we have UNUSED,\nwhich we should prefer, as it will let the compiler inform us if the\ncode changes to actually use the parameters.\n\nI also moved the annotations to come after the variable name, which is\nhow we typically spell it.\n\nSigned-off-by: Jeff King \n---\nWhere \"how we typically spell it\" is \"me\", because I wrote 99% of the\nannotations we have. ;) I'm open to debate, but only if it is\naccompanied by a patch to change all of them to be consistent.\n\n grep.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/grep.c b/grep.c\nindex 2f8b9553df..e5761426e4 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -245,7 +245,7 @@ static int is_fixed(const char *s, size_t len)\n #ifdef USE_LIBPCRE2\n #define GREP_PCRE2_DEBUG_MALLOC 0\n \n-static void *pcre2_malloc(PCRE2_SIZE size, MAYBE_UNUSED void *memory_data)\n+static void *pcre2_malloc(PCRE2_SIZE size, void *memory_data UNUSED)\n {\n \tvoid *pointer = malloc(size);\n #if GREP_PCRE2_DEBUG_MALLOC\n@@ -255,7 +255,7 @@ static void *pcre2_malloc(PCRE2_SIZE size, MAYBE_UNUSED void *memory_data)\n \treturn pointer;\n }\n \n-static void pcre2_free(void *pointer, MAYBE_UNUSED void *memory_data)\n+static void pcre2_free(void *pointer, void *memory_data UNUSED)\n {\n #if GREP_PCRE2_DEBUG_MALLOC\n \tstatic int count = 1;\n-- \n2.46.0.761.g18aface1ae\n"},{"id":"501870","messageId":"CAPig+cS0fT6+j3nQx7rh7bTDoMCsEUUWK3_YnpuiJVo+bVZWBQ@mail.gmail.com","threadId":"62020","inReplyTo":"20240829200953.GB432235@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] grep: prefer UNUSED to MAYBE_UNUSED for pcre allocators","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-08-29T20:27:41Z","receivedAt":"2024-08-29T20:27:53Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Aug 29, 2024 at 4:10 PM Jeff King wrote:\n> We prove custom malloc/free callbacks for the pcre library to use. Those\n\ns/prove/provide/\n\n> take an extra \"data\" parameter, but we don't use it. Back when these\n> were added in 513f2b0bbd (grep: make PCRE2 aware of custom allocator,\n> 2019-10-16), we only had MAYBE_UNUSED. But these days we have UNUSED,\n> which we should prefer, as it will let the compiler inform us if the\n> code changes to actually use the parameters.\n>\n> I also moved the annotations to come after the variable name, which is\n> how we typically spell it.\n>\n> Signed-off-by: Jeff King \n"},{"id":"501872","messageId":"xmqq34mnukff.fsf@gitster.g","threadId":"62020","inReplyTo":"20240829200807.GA430283@coredump.intra.peff.net","subject":"Re: [PATCH 0/2] clean up some MAYBE_UNUSED cases","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-29T20:46:12Z","receivedAt":"2024-08-29T20:46:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> - builtin/gc.c's check_crontab_process(). The whole function is marked\n> as MAYBE_UNUSED here, which is a little funny. It's because\n> is_crontab_available() may or may not call us based on __APPLE__.\n> Should we conditionally define the function, too, in that case? It\n> would mean repeating the #ifdef. Alternatively, we could define it\n> like this:\n>\n> #ifdef __APPLE__\n> static int check_crontab_process(const char *cmd UNUSED)\n> {\n> return 0;\n> }\n> #else\n> static int check_crontab_process(const char *cmd UNUSED)\n> {\n> [...the real function...]\n> }\n> #endif\n\nOr inline the body of check_crontab_process() at its sole callsite\n(the other side of \"#ifdef APPLE\") in is_crontab_available() and get\nrid of check_crontab_process().\n\n> But I think we're getting into \"well, this is how I would have\n> written it\" territory, and it doesn't matter much either way in\n> practice. It's probably better to just leave it alone.\n\nOK.\n"},{"id":"501890","messageId":"ZtFpGjBFlMNPnSld@tanuki","threadId":"62020","inReplyTo":"20240829200953.GB432235@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] grep: prefer UNUSED to MAYBE_UNUSED for pcre allocators","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-08-30T06:39:22Z","receivedAt":"2024-08-30T06:39:28Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Aug 29, 2024 at 04:09:53PM -0400, Jeff King wrote:\n> We prove custom malloc/free callbacks for the pcre library to use. Those\n> take an extra \"data\" parameter, but we don't use it. Back when these\n> were added in 513f2b0bbd (grep: make PCRE2 aware of custom allocator,\n> 2019-10-16), we only had MAYBE_UNUSED. But these days we have UNUSED,\n> which we should prefer, as it will let the compiler inform us if the\n> code changes to actually use the parameters.\n> \n> I also moved the annotations to come after the variable name, which is\n> how we typically spell it.\n> \n> Signed-off-by: Jeff King \n> ---\n> Where \"how we typically spell it\" is \"me\", because I wrote 99% of the\n> annotations we have. ;) I'm open to debate, but only if it is\n> accompanied by a patch to change all of them to be consistent.\n\nI don't care about the order, but if we settle on one I think we should\nalso document this accordingly in our code style guide.\n\nIn any case, the patch series looks obviously good, except for the one\ntypo that Eric already pointed out.\n\nThanks!\n\nPatrick\n"},{"id":"501925","messageId":"xmqqwmjyrn1l.fsf@gitster.g","threadId":"62020","inReplyTo":"ZtFpGjBFlMNPnSld@tanuki","subject":"Re: [PATCH 2/2] grep: prefer UNUSED to MAYBE_UNUSED for pcre allocators","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-30T16:30:14Z","receivedAt":"2024-08-30T16:30:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> On Thu, Aug 29, 2024 at 04:09:53PM -0400, Jeff King wrote:\n>> We prove custom malloc/free callbacks for the pcre library to use. Those\n>> take an extra \"data\" parameter, but we don't use it. Back when these\n>> were added in 513f2b0bbd (grep: make PCRE2 aware of custom allocator,\n>> 2019-10-16), we only had MAYBE_UNUSED. But these days we have UNUSED,\n>> which we should prefer, as it will let the compiler inform us if the\n>> code changes to actually use the parameters.\n>> \n>> I also moved the annotations to come after the variable name, which is\n>> how we typically spell it.\n>> \n>> Signed-off-by: Jeff King \n>> ---\n>> Where \"how we typically spell it\" is \"me\", because I wrote 99% of the\n>> annotations we have. ;) I'm open to debate, but only if it is\n>> accompanied by a patch to change all of them to be consistent.\n>\n> I don't care about the order, but if we settle on one I think we should\n> also document this accordingly in our code style guide.\n\nVery true. I think Peff's [PATCH 7/6] was sufficient by ending the\nnew instruction with\n\n\t... like \"int foo UNUSED\".\n\nwhen it talked about the -Werror=unused-parameter.\n\nThanks.\n"}]}