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

From: Derrick Stolee <derrickstolee@github.com>
Date: Aug 19, 2022, 18:59 UTC
Message-ID: <c22a8317-7d43-d84b-f63f-df2da31b4658@github.com>
In-Reply-To: <220819.861qtc8gug.gmgdl@evledraar.gmail.com>

On 8/19/2022 9:58 AM, Ævar Arnfjörð Bjarmason wrote:

Show 26 quoted lines

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

That's not the reason for the attribute at all. It's supposed to say "I know this is unused, but I still need it to be in the parameter list for other reasons. Don't create a warning for this case."

Interpreting it the way you are means "don't do the analysis. Just throw a warning." which doesn't make any sense.

Show 17 quoted lines

> 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

Does the deprecated attribute imply unused? Or at the very least, does it avoid the -Wunused-parameter warnings?

It might be helpful to _also_ have a deprecated annotation so we know to remove the UNUSED macro if a parameter starts being used again. The existing macro changes the variable name so we would get compiler errors if we started using it, but we could have a better message indicating exactly why things are not working.

So in that sense, you are onto something. Should we use both attributes?

At the very least, the warning message you recommend in the 'deprecated'
attribute could be more direct about what we expect.
	 
	 #if defined(__GNUC__)
	-#define UNUSED(var) UNUSED_##var __attribute__((unused))
	+#define UNUSED(var) var __attribute__((unused)) \
				 __attribute__((deprecated ("remove UNUSED macro before using")))
	 #else
	-#define UNUSED(var) UNUSED_##var
	+#define UNUSED(var) var
	 #endif
	 
	 #if defined(WIN32) && !defined(__CYGWIN__) /* Both MinGW and MSVC */

Thanks, -Stolee