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

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!")))
	-#define UNUSED(var) UNUSED_##var
	+#define UNUSED(var) var
	 #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.

  parent reply	other threads:[~2022-08-19 14:10 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 ` Ævar Arnfjörð Bjarmason [this message]
2022-08-19 18:59   ` [PATCH 0/11] annotating unused function parameters Derrick Stolee
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).