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

From: Jeff King <peff@peff.net>
Date: Aug 26, 2022, 07:10 UTC
Message-ID: <Ywhx6LLe6YcS/2xf@coredump.intra.peff.net>
In-Reply-To: <220825.86edx44lzh.gmgdl@evledraar.gmail.com>

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

Show 9 quoted lines

> > 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.

Show 7 quoted lines

> 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.

Show 8 quoted lines

> > 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