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

[Jeff King <peff@peff.net>]

On Thu, Aug 25, 2022 at 01:00:19PM +0200, Ævar Arnfjörð Bjarmason wrote:

> > One, it feels like we're abusing the deprecated attribute here. The
> 
> Definitely, but structurally it seems like a better pick. I.e. isn't the
> only problem with it the "deprecated" and its interaction with
> -Wno-deprecated.
> 
> If the exact same feature existed as a "insert-custom-warning", which
> would work exactly "deprecated" without the default warning "prefix"
> would you think this would fit perfectly?

Yeah, I agree that would remove my complaint about overloading the
meaning. I don't think that exists, though.

> This is mildly annoying, but I don't really think it's a practical
> issue. We're talking about running this without
> -Wno-deprecated-declarations in CI, and by default.
> 
> For unused parameters it's enough that we're catching them somewhere, or
> in common compilation settings, we don't need to catch them
> *everywhere*, do we?

No, but the farther away you go from the edit-compile-run cycle, the
more painful warnings become. Catching them immediately and fully has
real value, as it means the cost of correcting them is lower. So all
things being equal, I think we should prefer universal solutions when
they're available (and for example compiler errors over say, coccinelle
or other analysis tools).

(And yes, I know all things sadly aren't equal; see below...)

> IOW is anyone writing patches where they're testing with
> -Wno-deprecated-declarations *and* adding unused parameters *and* won't
> test without -Wno-deprecated-declarations before submitting them, *and*
> nobody else will catch it?

Probably not. I don't actually build with -Wno-deprecated-declarations
routinely. But my fear is that some platform may be stuck there for a
while (because an overzealous libc marks something). But that's kind of
hypothetical, so we may have to just accept it and cross that bridge if
we come to it.

> > 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))
> 
> ...and now to the real reason for the follow-up. You/Junio were CC'd,
> but this is breaking coccinelle, see:
> https://lore.kernel.org/git/220825.86ilmg4mil.gmgdl@evledraar.gmail.com/

Ugh. Yeah, that is really unfortunate. I much prefer the parenthesized
syntax, but if we can't find a way to unconfuse third-party parsing,
then switching is probably the least-bad solution.

-Peff