Re: [PATCH 0/11] annotating unused function parameters

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Aug 19, 2022, 13:58 UTC
Message-ID: <220819.861qtc8gug.gmgdl@evledraar.gmail.com>
In-Reply-To: <Yv9gxqH6nK2KYnNj@coredump.intra.peff.net>

On Fri, Aug 19 2022, Jeff King wrote:

Show 12 quoted lines

> I've been carrying a bunch of patches (for almost 4 years now!) that get
> the code base compiling cleanly with -Wunused-parameter. This is a
> useful warning in my opinion; it found real bugs[1] when applied to the
> whole code base. So it would be nice to be able to turn it on all the
> time and get the same protection going forward.
> [...]
> And of course the most important question is: do we like this direction
> overall. This mass-annotation is a one-time pain. Going forward, the
> only work would be requiring people to annotate new functions they add
> (which again, is mostly going to be callbacks). IMHO it's worth it. In
> addition to possibly finding errors, I think the annotations serve as an
> extra clue for people reading the code about what the author intended.

I've known you've had this out-of-tree for a while, and really like that it's on the path to getting integrated.

But I have a hang-up about it, which is that I though __attribute__ (unused) didn't work like *that*.

What it means (and maybe only I find this counter-intuitive) is "trust me, this is unused, but don't check!", furthermore it causes the compiler to completely ignore the variable for the purposes of *all* warnings, not just the unused one.

I may still be missing something, but I wonder if this squashed in
wouldn't be much better:
	
	diff --git a/git-compat-util.h b/git-compat-util.h
	index a9690126bb0..e02e2fc3f6d 100644
	--- a/git-compat-util.h
	+++ b/git-compat-util.h
	@@ -190,9 +190,9 @@ struct strbuf;
	 #define _SGI_SOURCE 1
	 
	 #if defined(__GNUC__)
	-#define UNUSED(var) UNUSED_##var __attribute__((unused))
	+#define UNUSED(var) var __attribute__((deprecated ("not 'deprecated', but expected not to be used!")))
	 #else
	-#define UNUSED(var) UNUSED_##var
	+#define UNUSED(var) var
	 #endif
	 
	 #if defined(WIN32) && !defined(__CYGWIN__) /* Both MinGW and MSVC */

I.e. it's a bit counter-intuitive to mark these as "deprecated", but you can add a custom message (with both GCC and clang). Improvements to the message welcome.

Now as in this series we stay silent if the variable is not used, but we
*don't* stay silent if an UNUSED(var) is actually used, that'll now be
an error:
	
	xdiff/xdiffi.c: In function ‘xdl_call_hunk_func’:
	xdiff/xdiffi.c:981:9: error: ‘xe’ is deprecated: not 'deprecated', but expected not to be used! [-Werror=deprecated-declarations]
	  981 |         fprintf(stderr, "%p", (void*)xe);
	      |         ^~~~~~~

This also means that you don't need to rename the variable just to avoid "accidental" use, which also has the benefit of not tripping up the variable typo detection.