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

From: Phillip Wood <phillip.wood123@gmail.com>
To: Jeff King <peff@peff.net>
Cc: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
	"Derrick Stolee" <derrickstolee@github.com>,
	"Git Mailing List" <git@vger.kernel.org>
Subject: Re: [PATCH 0/11] annotating unused function parameters
Date: Fri, 26 Aug 2022 14:08:44 +0100	[thread overview]
Message-ID: <CAPoeCOb8=6_7RCw6g-B8m_PsQkvYq6QDsk6Vu1WGiiZFG47K1w@mail.gmail.com> (raw)
In-Reply-To: <Ywhx6LLe6YcS/2xf@coredump.intra.peff.net>

Hi Peff

On Fri, 26 Aug 2022 at 08:33, Jeff King <peff@peff.net> wrote:
>
> 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.
> > 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).

That's a good point, one of the nice things about your macro was that
all compilers detected when UNUSED() parameters were in fact used. In
comparison abusing the deprecated attribute is uglier and less
practical.

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

It might be worth reporting this issue on the coccinelle mailing list
to see if anyone is interested in fixing it. If it gets fixed we're
left with the problem of having  to build it for our ci but that
shouldn't be insurmountable.

Best Wishes

Phillip

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