[PATCH 7/6] CodingGuidelines: mention -Wunused-parameter and UNUSED

From: Jeff King <peff@peff.net>
Date: Aug 28, 2024, 14:48 UTC
Message-ID: <20240828144814.GB4020916@coredump.intra.peff.net>
In-Reply-To: <CAPig+cQLr+vAzkt8UJNVCeE8osGEcEfFunG36oqxa0k8JamJzQ@mail.gmail.com>

On Wed, Aug 28, 2024 at 01:56:13AM -0400, Eric Sunshine wrote:

Show 12 quoted lines

> > 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.
> [...]
> 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?

Yeah, I agree some guidance is in order. How about this on top? I didn't go into the decision tree of when you should remove the parameter versus using it versus annotating it. I think in general that the first two are pretty obvious when they are appropriate, and we just need to focus on the annotating option.

-- >8 --
Subject: [PATCH] CodingGuidelines: mention -Wunused-parameter and UNUSED

Now that -Wunused-parameter is on by default for DEVELOPER=1 builds, people may trigger it, blocking their build. When it's a mistake for the parameter to exist, the path forward is obvious: remove it. But sometimes you need to suppress the warning, and the "UNUSED" mechanism for that is specific to our project, so people may not know about it.

Let's put some advice in CodingGuidelines, including an example warning message. That should help people who grep for the warning text after seeing it from the compiler.

Signed-off-by: Jeff King <peff@peff.net>
---
 Documentation/CodingGuidelines | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
index ccaea39752..d0fc7cfe60 100644
--- a/Documentation/CodingGuidelines
+++ b/Documentation/CodingGuidelines
@@ -258,6 +258,13 @@ For C programs:
    ensure your patch is clear of all compiler warnings we care about,
    by e.g. "echo DEVELOPER=1 >>config.mak".
 
+ - When using DEVELOPER=1 mode, you may see warnings from the compiler
+   like "error: unused parameter 'foo' [-Werror=unused-parameter]",
+   which indicates that a function ignores its argument. If the unused
+   parameter can't be removed (e.g., because the function is used as a
+   callback and has to match a certain interface), you can annotate the
+   individual parameters with the UNUSED keyword, like "int foo UNUSED".
+
  - We try to support a wide range of C compilers to compile Git with,
    including old ones.  As of Git v2.35.0 Git requires C99 (we check
    "__STDC_VERSION__"). You should not use features from a newer C

-- 
2.46.0.754.g24c813f009