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

On Fri, Aug 19, 2022 at 10:58:08PM +0200, Ævar Arnfjörð Bjarmason wrote:

Right. Using ((deprecated)) is really just a substitute for the variable renaming part.

And I agree it works as advertised, though I think I prefer the variable-renaming version.

One, it feels like we're abusing the deprecated attribute here. The confusion in the compiler output I'm OK with, because we get a chance to put our own message there (so I agree the output is actually better than with my patch). But from time to time I've had to build with -Wno-deprecated-declarations to get around _actual_ deprecated warnings (e.g., compiling with OPENSSL_SHA1=Yes). And doing so would be cutting out half the protection of UNUSED() in that case.

Likewise, one thing I like about the renaming is that it fails compilation regardless of -Werror. So it will be caught in any compile, no matter what. And I do automatically compile without DEVELOPER=1 when on a detached HEAD, because historical commits often trigger warnings. Go back far enough and OPENSSL_SHA1 was the default, which generates lots of warnings these days. :)

And finally, I actually prefer the parentheses of:

  static int register_ref(const char *refname, const struct object_id *oid,
			  int UNUSED(flags), void *UNUSED(cb_data))

It visually binds the attribute more tightly to the variable name, like how we sometimes write unused_flags manually in existing code. When there are a lot of variables to mark in a function (and some callbacks really do have a lot), that makes it easier to see what's going on. To me, anyway. I recognize that it's a matter of taste.

Technically this last thing is orthogonal to using the deprecated attribute. It could still be used with the parenthesized form, but the error messages gcc generates are horrendous then (it repeats the warning several times due to the macro).

So I dunno. These are all matters of opinion, and if it was just my patches, I'd say my taste wins. But all of us are going to have to write these annotations at some time or another when we add callbacks, etc. So we should at the very least pick a syntax the majority prefers. :)

-Peff