git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jeff King <peff@peff.net>
To: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Cc: Derrick Stolee <derrickstolee@github.com>, git@vger.kernel.org
Subject: Re: [PATCH 0/11] annotating unused function parameters
Date: Sat, 20 Aug 2022 05:46:59 -0400	[thread overview]
Message-ID: <YwCtkwjWdJVHHZV0@coredump.intra.peff.net> (raw)
In-Reply-To: <220819.868rnk54ju.gmgdl@evledraar.gmail.com>

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

> Yes, I spoke too soon, sorry. We still need ((unused)). FWIW the below
> on top of master and doing:

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

  reply	other threads:[~2022-08-20  9:47 UTC|newest]

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-08-19 10:07 [PATCH 0/11] annotating unused function parameters Jeff King
2022-08-19 10:08 ` [PATCH 01/11] git-compat-util: add UNUSED macro Jeff King
2022-08-19 10:08 ` [PATCH 02/11] refs: mark unused each_ref_fn parameters Jeff King
2022-08-19 10:08 ` [PATCH 03/11] refs: mark unused reflog callback parameters Jeff King
2022-08-19 10:08 ` [PATCH 04/11] refs: mark unused virtual method parameters Jeff King
2022-08-19 10:08 ` [PATCH 05/11] transport: mark bundle transport_options as unused Jeff King
2022-08-19 10:08 ` [PATCH 06/11] streaming: mark unused virtual method parameters Jeff King
2022-08-19 10:08 ` [PATCH 07/11] config: mark unused callback parameters Jeff King
2022-08-19 10:08 ` [PATCH 08/11] hashmap: " Jeff King
2022-08-19 10:08 ` [PATCH 09/11] mark unused read_tree_recursive() " Jeff King
2022-08-19 10:08 ` [PATCH 10/11] run-command: mark unused async " Jeff King
2022-08-19 10:08 ` [PATCH 11/11] is_path_owned_by_current_uid(): mark "report" parameter as unused Jeff King
2022-08-19 13:58 ` [PATCH 0/11] annotating unused function parameters Ævar Arnfjörð Bjarmason
2022-08-19 18:59   ` Derrick Stolee
2022-08-19 20:58     ` Ævar Arnfjörð Bjarmason
2022-08-20  9:46       ` Jeff King [this message]
2022-08-20 21:21         ` Junio C Hamano
2022-08-22 14:14         ` Derrick Stolee
2022-08-25 11:00         ` Ævar Arnfjörð Bjarmason
2022-08-26  7:10           ` Jeff King
2022-08-26 13:08             ` Phillip Wood
2022-08-26 16:37             ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YwCtkwjWdJVHHZV0@coredump.intra.peff.net \
    --to=peff@peff.net \
    --cc=avarab@gmail.com \
    --cc=derrickstolee@github.com \
    --cc=git@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).