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.
