Re: [PATCH 4/3] pack-bitmap.h: fix unused variable warning

From: Jeff King <peff@peff.net>
To: Denton Liu <liu.denton@gmail.com>
Cc: Git Mailing List <git@vger.kernel.org>
Subject: Re: [PATCH 4/3] pack-bitmap.h: fix unused variable warning
Date: Tue, 24 Sep 2019 17:34:08 -0400	[thread overview]
Message-ID: <20190924213408.GD20858@sigill.intra.peff.net> (raw)
In-Reply-To: <a3a3357925cb144deb35b298cbe2796934fa0673.1569315905.git.liu.denton@gmail.com>

On Tue, Sep 24, 2019 at 02:08:53AM -0700, Denton Liu wrote:

> When we ran `make hdr-check`, we got the following warning on Arch Linux:
> 
> 	pack-bitmap.h:20:19: error: ‘BITMAP_IDX_SIGNATURE’ defined but not used [-Werror=unused-const-variable=]
> 	   20 | static const char BITMAP_IDX_SIGNATURE[] = {'B', 'I', 'T', 'M'};
> 	      |                   ^~~~~~~~~~~~~~~~~~~~
> 	cc1: all warnings being treated as errors
> 
> "Use" the BITMAP_IDX_SIGNATURE variable by making the size of
> bitmap_disk_header.magic equal to the size of BITMAP_IDX_SIGNATURE. An
> alternative was to simply add MAYBE_UNUSED. However, this design was
> chosen because we eliminate the magic number (4) in the process.

Yeah, I think this is a fine fix. Since it's needed in only a few files,
the more usual fix would be to not define it in the header in the first
place. Something like:

  extern const char BITMAP_IDX_SIGNATURE[4];

or whatever. But I think it's nice to keep it all together.

> I'm tacking this patch on since this warning didn't show up until I
> compiled it on gcc 9.1.0.

Curiously, I _don't_ see the warning with gcc 9.2.1. By my reading of
the manpage, this should be triggered by -Wunused-const-variable=2, but
not by "1" (the difference being whether it triggers for stuff in header
files). And only the latter is triggered by -Wall or -Wextra.

But another weirdness is that hdr-check is directly compiling the header
files. So I guess that fools it. But we don't pass any of the extra
diagnostic options there.  Have you put "-Wall" into your $(CC)?

Perhaps a more realistic hdr-check would be:

  {
    echo '#include "git-compat-util.h"'
    echo '#include "$<"'
  } >$*.hcc
  $(QUIET_HDR)$(CC) $(ALL_CFLAGS) -o /dev/null -c -xc $*.hcc

-Peff