[PATCH 0/2] clean up some MAYBE_UNUSED cases

[PATCH 0/2] clean up some MAYBE_UNUSED cases

[Jeff King]

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

[PATCH 1/2] gc: drop MAYBE_UNUSED annotation from used parameter

[Jeff King]

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 <peff@peff.net>
---
 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

[PATCH 2/2] grep: prefer UNUSED to MAYBE_UNUSED for pcre allocators

[Jeff King]

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 <peff@peff.net>
---
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

Re: [PATCH 2/2] grep: prefer UNUSED to MAYBE_UNUSED for pcre allocators

[Eric Sunshine]

On Thu, Aug 29, 2024 at 4:10 PM Jeff King <peff@peff.net> 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 <peff@peff.net>

Re: [PATCH 2/2] grep: prefer UNUSED to MAYBE_UNUSED for pcre allocators

[Patrick Steinhardt]

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 <peff@peff.net>
> ---
> 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

Re: [PATCH 2/2] grep: prefer UNUSED to MAYBE_UNUSED for pcre allocators

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> 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 <peff@peff.net>
>> ---
>> 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.

Re: [PATCH 0/2] clean up some MAYBE_UNUSED cases

[Junio C Hamano]

Jeff King <peff@peff.net> 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.