archive mirror
 help / color / mirror / Atom feed
From: Derrick Stolee <>
To: "Ævar Arnfjörð Bjarmason" <>,
	"Jeff King" <>
Subject: Re: [PATCH 0/11] annotating unused function parameters
Date: Fri, 19 Aug 2022 14:59:35 -0400	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <>

On 8/19/2022 9:58 AM, Ævar Arnfjörð Bjarmason wrote:
> 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.

> 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")))
	-#define UNUSED(var) UNUSED_##var
	+#define UNUSED(var) var
	 #if defined(WIN32) && !defined(__CYGWIN__) /* Both MinGW and MSVC */


  reply	other threads:[~2022-08-19 18:59 UTC|newest]

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-08-19 10:07 [PATCH 0/11] annotating unused function parameters Jeff King
2022-08-19 10:08 ` [PATCH 01/11] git-compat-util: add UNUSED macro Jeff King
2022-08-19 10:08 ` [PATCH 02/11] refs: mark unused each_ref_fn parameters Jeff King
2022-08-19 10:08 ` [PATCH 03/11] refs: mark unused reflog callback parameters Jeff King
2022-08-19 10:08 ` [PATCH 04/11] refs: mark unused virtual method parameters Jeff King
2022-08-19 10:08 ` [PATCH 05/11] transport: mark bundle transport_options as unused Jeff King
2022-08-19 10:08 ` [PATCH 06/11] streaming: mark unused virtual method parameters Jeff King
2022-08-19 10:08 ` [PATCH 07/11] config: mark unused callback parameters Jeff King
2022-08-19 10:08 ` [PATCH 08/11] hashmap: " Jeff King
2022-08-19 10:08 ` [PATCH 09/11] mark unused read_tree_recursive() " Jeff King
2022-08-19 10:08 ` [PATCH 10/11] run-command: mark unused async " Jeff King
2022-08-19 10:08 ` [PATCH 11/11] is_path_owned_by_current_uid(): mark "report" parameter as unused Jeff King
2022-08-19 13:58 ` [PATCH 0/11] annotating unused function parameters Ævar Arnfjörð Bjarmason
2022-08-19 18:59   ` Derrick Stolee [this message]
2022-08-19 20:58     ` Ævar Arnfjörð Bjarmason
2022-08-20  9:46       ` Jeff King
2022-08-20 21:21         ` Junio C Hamano
2022-08-22 14:14         ` Derrick Stolee
2022-08-25 11:00         ` Ævar Arnfjörð Bjarmason
2022-08-26  7:10           ` Jeff King
2022-08-26 13:08             ` Phillip Wood
2022-08-26 16:37             ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \ \ \ \ \ \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).