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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Fri, Aug 19 2022, Jeff King wrote:

> 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.