Re: [PATCH] Makefile: drop GEN_HDRS

[Jeff King <peff@peff.net>]

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

> > 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.

> > 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).

> 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