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

Commit Message

Jeff King Aug. 28, 2024, 2:48 p.m. UTC

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

> > 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 <[e-mail]>
---
 Documentation/CodingGuidelines | 7 +++++++
 1 file changed, 7 insertions(+)

Comments

Eric Sunshine Aug. 28, 2024, 7:53 p.m. UTC | #1

On Wed, Aug 28, 2024 at 10:48 AM Jeff King <[e-mail]> wrote:
> On Wed, Aug 28, 2024 at 01:56:13AM -0400, Eric Sunshine wrote:
> > 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.

Makes sense.

> Signed-off-by: Jeff King <[e-mail]>
> ---
> diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
> @@ -258,6 +258,13 @@ For C programs:
> + - 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".

Perfect. This fully addresses the question expressed by my review
comment. Thank you.

Patch

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