# [PATCH 0/2] clean up some MAYBE_UNUSED cases 7 messages from 2024-08-29 to 2024-08-30. Participants: Jeff King, Eric Sunshine, Junio C Hamano, Patrick Steinhardt. Thread: https://gitlist.dev/t/62020 ## Jeff King, 2024-08-29 20:08 Subject: [PATCH 0/2] clean up some MAYBE_UNUSED cases Message-ID: <20240829200807.GA430283@coredump.intra.peff.net> URL: https://gitlist.dev/e/20240829200807.GA430283%40coredump.intra.peff.net ``` The discussion about MAYBE_UNUSED over in[1] made me wonder how it was used in practice. This series cleans up a few spots where I think it is being misused. [1] https://lore.kernel.org/git/xmqqseunxtks.fsf_-_@gitster.g/ The remaining grep hits are: - fsmonitor has a few functions with sometimes-unused parameters based on conditional compilation (try_to_run_foreground_daemon() and check_for_incompatible()). This is a good use of MAYBE_UNUSED. - builtin/gc.c's check_crontab_process(). The whole function is marked as MAYBE_UNUSED here, which is a little funny. It's because is_crontab_available() may or may not call us based on __APPLE__. Should we conditionally define the function, too, in that case? It would mean repeating the #ifdef. Alternatively, we could define it like this: #ifdef __APPLE__ static int check_crontab_process(const char *cmd UNUSED) { return 0; } #else static int check_crontab_process(const char *cmd UNUSED) { [...the real function...] } #endif But I think we're getting into "well, this is how I would have written it" territory, and it doesn't matter much either way in practice. It's probably better to just leave it alone. - commit-slab marks auto-generated static functions with MAYBE_UNUSED, since it doesn't know which ones actually need to be instantiated. This was the original thing we added MAYBE_UNUSED for. - khash does something similar when auto-generating functions. - test_bitmap_commits() marks a local variable as MAYBE_UNUSED! In fact it's completely unused by the function itself, but the macro expansion of kh_foreach() assigns to it. So we need the variable to exist to pass into the macro, but the compiler warning is triggered on the expanded code. We have kh_foreach_value(), but not kh_foreach_key(), which is what we'd want here. It wouldn't be hard to add it, but the MAYBE_UNUSED here is doing a fine job of suppressing the warning (and presumably an optimizing compiler removes the useless assignment). I prepared this on top of what's queued in jk/unused-parameters (which helps with making sure the annotations are all correct), but it could be applied separately. [1/2]: gc: drop MAYBE_UNUSED annotation from used parameter [2/2]: grep: prefer UNUSED to MAYBE_UNUSED for pcre allocators builtin/gc.c | 2 +- grep.c | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) -Peff ``` ## Jeff King, 2024-08-29 20:08 Subject: [PATCH 1/2] gc: drop MAYBE_UNUSED annotation from used parameter Message-ID: <20240829200833.GA432235@coredump.intra.peff.net> URL: https://gitlist.dev/e/20240829200833.GA432235%40coredump.intra.peff.net In-Reply-To: <20240829200807.GA430283@coredump.intra.peff.net> ``` The "opts" parameter is always used, so marking it with MAYBE_UNUSED is just confusing. This annotation goes back to 41abfe15d9 (maintenance: add pack-refs task, 2021-02-09), when it really was unused. Back then we did not have the UNUSED macro that would complain if the code changed to use the parameter. So when we started using it in bfc2f9eb8e (builtin/gc: forward git-gc(1)'s `--auto` flag when packing refs, 2024-03-25), nobody noticed. Signed-off-by: Jeff King --- builtin/gc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/gc.c b/builtin/gc.c index 0e361253e3..7dac971405 100644 --- a/builtin/gc.c +++ b/builtin/gc.c @@ -260,7 +260,7 @@ static int pack_refs_condition(UNUSED struct gc_config *cfg) return 1; } -static int maintenance_task_pack_refs(MAYBE_UNUSED struct maintenance_run_opts *opts, +static int maintenance_task_pack_refs(struct maintenance_run_opts *opts, UNUSED struct gc_config *cfg) { struct child_process cmd = CHILD_PROCESS_INIT; -- 2.46.0.761.g18aface1ae ``` ## Jeff King, 2024-08-29 20:09 Subject: [PATCH 2/2] grep: prefer UNUSED to MAYBE_UNUSED for pcre allocators Message-ID: <20240829200953.GB432235@coredump.intra.peff.net> URL: https://gitlist.dev/e/20240829200953.GB432235%40coredump.intra.peff.net In-Reply-To: <20240829200807.GA430283@coredump.intra.peff.net> ``` We prove custom malloc/free callbacks for the pcre library to use. Those take an extra "data" parameter, but we don't use it. Back when these were added in 513f2b0bbd (grep: make PCRE2 aware of custom allocator, 2019-10-16), we only had MAYBE_UNUSED. But these days we have UNUSED, which we should prefer, as it will let the compiler inform us if the code changes to actually use the parameters. I also moved the annotations to come after the variable name, which is how we typically spell it. Signed-off-by: Jeff King --- Where "how we typically spell it" is "me", because I wrote 99% of the annotations we have. ;) I'm open to debate, but only if it is accompanied by a patch to change all of them to be consistent. grep.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/grep.c b/grep.c index 2f8b9553df..e5761426e4 100644 --- a/grep.c +++ b/grep.c @@ -245,7 +245,7 @@ static int is_fixed(const char *s, size_t len) #ifdef USE_LIBPCRE2 #define GREP_PCRE2_DEBUG_MALLOC 0 -static void *pcre2_malloc(PCRE2_SIZE size, MAYBE_UNUSED void *memory_data) +static void *pcre2_malloc(PCRE2_SIZE size, void *memory_data UNUSED) { void *pointer = malloc(size); #if GREP_PCRE2_DEBUG_MALLOC @@ -255,7 +255,7 @@ static void *pcre2_malloc(PCRE2_SIZE size, MAYBE_UNUSED void *memory_data) return pointer; } -static void pcre2_free(void *pointer, MAYBE_UNUSED void *memory_data) +static void pcre2_free(void *pointer, void *memory_data UNUSED) { #if GREP_PCRE2_DEBUG_MALLOC static int count = 1; -- 2.46.0.761.g18aface1ae ``` ## Eric Sunshine, 2024-08-29 20:27 Subject: Re: [PATCH 2/2] grep: prefer UNUSED to MAYBE_UNUSED for pcre allocators Message-ID: URL: https://gitlist.dev/e/CAPig%2BcS0fT6%2Bj3nQx7rh7bTDoMCsEUUWK3_YnpuiJVo%2BbVZWBQ%40mail.gmail.com In-Reply-To: <20240829200953.GB432235@coredump.intra.peff.net> ``` On Thu, Aug 29, 2024 at 4:10 PM Jeff King wrote: > We prove custom malloc/free callbacks for the pcre library to use. Those s/prove/provide/ > take an extra "data" parameter, but we don't use it. Back when these > were added in 513f2b0bbd (grep: make PCRE2 aware of custom allocator, > 2019-10-16), we only had MAYBE_UNUSED. But these days we have UNUSED, > which we should prefer, as it will let the compiler inform us if the > code changes to actually use the parameters. > > I also moved the annotations to come after the variable name, which is > how we typically spell it. > > Signed-off-by: Jeff King ``` ## Junio C Hamano, 2024-08-29 20:46 Subject: Re: [PATCH 0/2] clean up some MAYBE_UNUSED cases Message-ID: URL: https://gitlist.dev/e/xmqq34mnukff.fsf%40gitster.g In-Reply-To: <20240829200807.GA430283@coredump.intra.peff.net> ``` Jeff King writes: > - builtin/gc.c's check_crontab_process(). The whole function is marked > as MAYBE_UNUSED here, which is a little funny. It's because > is_crontab_available() may or may not call us based on __APPLE__. > Should we conditionally define the function, too, in that case? It > would mean repeating the #ifdef. Alternatively, we could define it > like this: > > #ifdef __APPLE__ > static int check_crontab_process(const char *cmd UNUSED) > { > return 0; > } > #else > static int check_crontab_process(const char *cmd UNUSED) > { > [...the real function...] > } > #endif Or inline the body of check_crontab_process() at its sole callsite (the other side of "#ifdef APPLE") in is_crontab_available() and get rid of check_crontab_process(). > But I think we're getting into "well, this is how I would have > written it" territory, and it doesn't matter much either way in > practice. It's probably better to just leave it alone. OK. ``` ## Patrick Steinhardt, 2024-08-30 06:39 Subject: Re: [PATCH 2/2] grep: prefer UNUSED to MAYBE_UNUSED for pcre allocators Message-ID: URL: https://gitlist.dev/e/ZtFpGjBFlMNPnSld%40tanuki In-Reply-To: <20240829200953.GB432235@coredump.intra.peff.net> ``` On Thu, Aug 29, 2024 at 04:09:53PM -0400, Jeff King wrote: > We prove custom malloc/free callbacks for the pcre library to use. Those > take an extra "data" parameter, but we don't use it. Back when these > were added in 513f2b0bbd (grep: make PCRE2 aware of custom allocator, > 2019-10-16), we only had MAYBE_UNUSED. But these days we have UNUSED, > which we should prefer, as it will let the compiler inform us if the > code changes to actually use the parameters. > > I also moved the annotations to come after the variable name, which is > how we typically spell it. > > Signed-off-by: Jeff King > --- > Where "how we typically spell it" is "me", because I wrote 99% of the > annotations we have. ;) I'm open to debate, but only if it is > accompanied by a patch to change all of them to be consistent. I don't care about the order, but if we settle on one I think we should also document this accordingly in our code style guide. In any case, the patch series looks obviously good, except for the one typo that Eric already pointed out. Thanks! Patrick ``` ## Junio C Hamano, 2024-08-30 16:30 Subject: Re: [PATCH 2/2] grep: prefer UNUSED to MAYBE_UNUSED for pcre allocators Message-ID: URL: https://gitlist.dev/e/xmqqwmjyrn1l.fsf%40gitster.g In-Reply-To: ``` Patrick Steinhardt writes: > On Thu, Aug 29, 2024 at 04:09:53PM -0400, Jeff King wrote: >> We prove custom malloc/free callbacks for the pcre library to use. Those >> take an extra "data" parameter, but we don't use it. Back when these >> were added in 513f2b0bbd (grep: make PCRE2 aware of custom allocator, >> 2019-10-16), we only had MAYBE_UNUSED. But these days we have UNUSED, >> which we should prefer, as it will let the compiler inform us if the >> code changes to actually use the parameters. >> >> I also moved the annotations to come after the variable name, which is >> how we typically spell it. >> >> Signed-off-by: Jeff King >> --- >> Where "how we typically spell it" is "me", because I wrote 99% of the >> annotations we have. ;) I'm open to debate, but only if it is >> accompanied by a patch to change all of them to be consistent. > > I don't care about the order, but if we settle on one I think we should > also document this accordingly in our code style guide. Very true. I think Peff's [PATCH 7/6] was sufficient by ending the new instruction with ... like "int foo UNUSED". when it talked about the -Werror=unused-parameter. Thanks. ```