From: Junio C Hamano <gitster@pobox•com>
To: Eric Sunshine <sunshine@sunshineco•com>
Cc: Jeff King <peff@peff•net>, git@vger•kernel.org
Subject: Re: [PATCH 6/6] config.mak.dev: enable -Wunused-parameter by default
Date: Wed, 28 Aug 2024 08:17:54 -0700 [thread overview]
Message-ID: <xmqqle0g4qxp.fsf@gitster.g> (raw)
In-Reply-To: <CAPig+cQLr+vAzkt8UJNVCeE8osGEcEfFunG36oqxa0k8JamJzQ@mail.gmail.com> (Eric Sunshine's message of "Wed, 28 Aug 2024 01:56:13 -0400")
Eric Sunshine <sunshine@sunshineco•com> writes:
>> 3. the function has to conform to a specific interface (because it's
>> used via a function pointer, or matches something on the other side
>> of an #ifdef). These ones are annoying, but annotating them with
>> UNUSED is not too bad (especially if the compiler tells you about
>> the problem promptly).
>> [...]
>> And since the code base is now at a spot where we compile cleanly with
>> -Wunused-parameter, turning it on will make it the responsibility of
>> individual patch writers going forward.
>>
>> Signed-off-by: Jeff King <peff@peff•net>
>> ---
>> diff --git a/config.mak.dev b/config.mak.dev
>> @@ -54,7 +54,6 @@ ifeq ($(filter extra-all,$(DEVOPTS)),)
>> DEVELOPER_CFLAGS += -Wno-sign-compare
>> -DEVELOPER_CFLAGS += -Wno-unused-parameter
>
> What is the expectation regarding newcomers to the project or even
> people who have not been following this topic and its cousins?
> Documentation/CodingGuidelines recommends enabling DEVELOPER mode,
> which is good, but this change means that such people may now be hit
> with a compiler complaint which they don't necessarily know how to
> deal with in the legitimate case #3 (described above). Should
> CodingGuidelines be updated to mention "UNUSED" and the circumstances
> under which it should be used?
I am not yet convinced 100%, but probably it is a good idea.
We have our idioms and conventions like use of UNLEAK(), UNUSED,
__attribute__ to annotate varargs functions that take printf-like
format, always using "{ 0 }" and no other form to zero-initialize an
auto variable of an aggregate type, etc., that are unreasonable for
somebody, who is new to the project but is fluent in and competent
at C, to know all of them.
For things like UNLEAK() that require a bit more than general
competence in the language, a bit more thought and peeking the
implementation to understand how they work, we should document them
to make it easier to learn for new people.
Consistently using "{ 0 }", instead of picking a single random
member and initialize the aggrevate with "{ .it = 0 }", is asking
the writer to pick between two _valid_ C language constructs and
always use one of them, so it may make sense to document it, even
though the general "do what the existing code does" may be
sufficient.
Unlike them, UNUSED smells a lot more obvious, and because the
code base is full of API functions that take callback functions,
we have plenty of existing uses of it for those who are competent
but are unfamiliar to the code base to notice and pick up quickly.
So I have a feeling that relatively speaking it is less necessary to
help new contributors with documenting UNUSED than other conventions
we have.
I have no objection if somebody else does a thoughtful job to
document these things evenly, but if we are going to document
UNUSED, we should explicitly document that it is our policy to
document each and every one of these conventions to help new
contributors.
Thanks.
next prev parent reply other threads:[~2024-08-28 15:18 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-08-28 3:57 [PATCH 0/6] unused parameters: the final countdown Jeff King
2024-08-28 3:57 ` [PATCH 1/6] gc: mark unused config parameter in virtual functions Jeff King
2024-08-28 3:57 ` [PATCH 2/6] t-reftable-readwrite: mark unused parameter in callback function Jeff King
2024-08-28 3:58 ` [PATCH 3/6] compat: disable -Wunused-parameter in 3rd-party code Jeff King
2024-08-28 3:59 ` [PATCH 4/6] compat: disable -Wunused-parameter in win32/headless.c Jeff King
2024-08-28 4:00 ` [PATCH 5/6] compat: mark unused parameters in win32/mingw functions Jeff King
2024-08-28 4:00 ` [PATCH 6/6] config.mak.dev: enable -Wunused-parameter by default Jeff King
2024-08-28 5:56 ` Eric Sunshine
2024-08-28 8:21 ` Patrick Steinhardt
2024-08-28 14:48 ` [PATCH 7/6] CodingGuidelines: mention -Wunused-parameter and UNUSED Jeff King
2024-08-28 19:53 ` Eric Sunshine
2024-08-28 15:17 ` Junio C Hamano [this message]
2024-08-28 4:12 ` [PATCH 0/6] unused parameters: the final countdown Jeff King
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=xmqqle0g4qxp.fsf@gitster.g \
--to=gitster@pobox$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=peff@peff$(echo .)net \
--cc=sunshine@sunshineco$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
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