Re: [PATCH] Makefile: drop GEN_HDRS

From: Jeff King <peff@peff.net>
Date: Dec 17, 2019, 05:22 UTC
Message-ID: <20191217052224.GA2762303@coredump.intra.peff.net>
In-Reply-To: <20191217012756.GQ135450@google.com>

On Mon, Dec 16, 2019 at 05:27:56PM -0800, Emily Shaffer wrote:

Show 11 quoted lines

> > Yeah, I don't think there's any change in behavior here, since with the
> > exception of hdr-check, every mention of $(LIB_H) also mentioned
> > $(GENERATED_H). And in the case of hdr-check, we explicitly exclude the
> > only item found in $(GENERATED_H).
> 
> To check my understanding - hdr-check just says "the headers are
> syntactically correct", right? $HCO's target '-o /dev/null' says
> "don't save the output", '-c' says "just compile, don't link", and '-xc'
> says "in C"; it expands to a target for each file ending in .h but not
> in $EXCEPT_HDRS, and hdr-check calls each one of those expanded targets,
> so I think I understand hdr-check is compiling each header...

Sort of. It's less about "syntactically correct" (which we'd find out easily when we try to compile it) and more about "does not have unexpected dependencies on other files".

E.g., imagine I write a header foo.h that mentions "struct bar", which is declared in bar.h. If the only C file that uses that does:

  #include "bar.h"
  #include "foo.h"

then the compiler is happy. But I've laid a trap for somebody later, who does just:

  #include "foo.h"

and gets an annoying compiler error (which is trivial to fix in this example, but can sometimes get complicated to untangle).

So we declared a rule that header files should be self-sufficient (modulo git-compat-util.h), and hdr-check is the way to find out if that is true.

Show 6 quoted lines

> > but I'm not sure if that is really turning up any useful problems. I
> > suppose somebody besides help.c could include command-list.h, in which
> > case some of those MAYBE_UNUSED bits could become useful.
> 
> Firstly, I think if someone besides help.c includes command-list.h it
> blows up because there's no include guards :)

Only if another header file does it, which could easily cause double inclusion within the same source file. It's perfectly fine for another translation unit to include it.

(Side note: builtin/help.c is declared in the Makefile as depending on it, but doesn't seem to actually include it).

Show 7 quoted lines

> My gut wants to say, "I need to be sure my generated header is correct!"
> But it seems that will also be checked when I try to include that header
> from something actually important. So maybe it's not actually useful.
> But then it seems like hdr-check target isn't that useful for anybody,
> since those headers should always be included sometime down the road (or
> why have them). So that makes me think I must still be missing
> something, like maybe I parsed hdr-check wrong.

I think this is the "it compiles now but you've laid a trap..." thing mentioned above.

As it is, command-list.h _does_ have such a trap; you need to include gettext.h first. But since so few callers use it (and are likely to use it) nobody has really noticed or cared.

-Peff