Re: [PATCH 1/9] Makefile: add a hdr-check target

["Martin Ågren" <martin.agren@gmail.com>]

On Wed, 19 Sep 2018 at 22:15, Ramsay Jones <ramsay@ramsayjones.plus.com> wrote:
> On 19/09/18 18:49, Martin Ågren wrote:
> > On Wed, 19 Sep 2018 at 02:07, Ramsay Jones <ramsay@ramsayjones.plus.com> wrote:
> >> +GEN_HDRS := command-list.h unicode-width.h
> >
> > Most of the things happening around here are obvious steps towards the
> > end-goal and seem to logically belong here, together. This one though,
> > "generated headers"(?) seems like it is more general in nature, so could
> > perhaps live somewhere else.
>
> Yes, generated headers, but no, not more general. ;-)

> The 'command-list.h' is generated as part of the build
> (and so may or may not be part of the LIB_H macro), whereas
> the unicode-width.h header is only re-generated when someone
> runs the 'contrib/update-unicode/update_unicode.sh' script
> (presumably when a new standard version is announced) and
> commits the result.

Ah, I wasn't aware of how unicode-width.h works, thanks.

> Both headers fail the 'hdr-check', although both generator
> scripts could be 'fixed' so that they passed. I just didn't
> think it was worth the effort - neither header was likely
> to be #included anywhere else.

With the above background, I'd tend to agree.

> I guess I could eliminate
> that macro, absorbing it into EXCEPT_HDRS, if that would
> lead to less confusion ...

I'm just a single data point, so don't trust my experience too much.

> > Actually, we have a variable `GENERATED_H` which seems to try to do more
> > or less the same thing. It lists just one file, though, command-list.h.
>
> Hmm, GENERATED_H seems only to be used by the i18n part of the
> makefile and, as a result of its appearance in LIB_H, sometimes
> results in command-list.h appearing twice in LOCALIZED_C.

Just thinking out loud, I suppose you could use $(GENERATED_H) instead
of hard-coding command-list.h in your patch. But with the background you
explained above, I think there's a good counter-argument to be made:

When we gain new auto-generated headers, we wouldn't actually mind `make
hdr-check` failing. We'd get the opportunity to decide whether making
the new header pass the check is worth it, or whether the correct
solution is to blacklist the auto-generated header. That's probably
better than just blacklisting the new header by default so that we don't
even notice that it has a potential problem.

So I think your approach makes sense. I stumbled on the similarity of
the name to something we already have. Maybe something like

  # Ignore various generated files, rather than updating the
  # generating scripts for little or no benefit.
  GEN_HDRS := ...

or a remark in the commit message, or rolling this into EXCEPT_HDRS, but
I'd be perfectly fine just leaving this as it is. Please trust your own
judgment more than mine.

Thanks
Martin