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

From: Jeff King <peff@peff.net>
Date: Sep 24, 2019, 21:34 UTC
Message-ID: <20190924213408.GD20858@sigill.intra.peff.net>
In-Reply-To: <a3a3357925cb144deb35b298cbe2796934fa0673.1569315905.git.liu.denton@gmail.com>

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

Show 11 quoted lines

> 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