public inbox for git@vger.kernel.org 
 help / color / mirror / Atom feed

From: Jonathan Nieder <jrnieder@gmail•com>
To: Emily Shaffer <emilyshaffer@google•com>
Cc: Jeff King <peff@peff•net>, Junio C Hamano <gitster@pobox•com>,
	git@vger•kernel.org, Ramsay Jones <ramsay@ramsayjones•plus.com>
Subject: Re: [PATCH] Makefile: drop GEN_HDRS
Date: Mon, 16 Dec 2019 17:40:57 -0800	[thread overview]
Message-ID: <20191217014057.GE10734@google.com> (raw)
In-Reply-To: <20191217012756.GQ135450@google.com>

Emily Shaffer wrote:
> On Mon, Dec 16, 2019 at 02:20:14PM -0500, Jeff King wrote:

>> But this would enable us to start checking command-list.h. I'm on the
>> fence on whether that's useful or not; the patch below makes it pass,
>> 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 :)
>
> 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.

"git log -Shdr-check Makefile" gives a hint:

  $ git log -Shdr-check Makefile
  commit ebb7baf02f69f2164b1f89148945d18c376fc6a8
  Author: Ramsay Jones <ramsay@ramsayjones•plus.com>
  Date:   Wed Sep 19 01:07:08 2018 +0100

      Makefile: add a hdr-check target

      Commit ef3ca95475 ("Add missing includes and forward declarations",
      2018-08-15) resulted from the author employing a manual method to
      create a C file consisting of a pair of pre-processor #include
      lines (for 'git-compat-util.h' and a given toplevel header), and
      fixing any resulting compiler errors or warnings.

      Add a Makefile target to automate this process.

So it's primarily about making sure that each header is #include-able
on its own (i.e., that it #includes the headers for any types it
relies on).

That seems like something I'd want to hold for command-list.h, too. :)

Thanks,
Jonathan

next prev parent reply	other threads:[~2019-12-17  1:41 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-12-13 23:25 [PATCH] Makefile: drop GEN_HDRS Junio C Hamano
2019-12-14  0:38 ` Jeff King
2019-12-14  1:00   ` Jeff King
2019-12-16 18:55     ` Junio C Hamano
2019-12-16 19:20       ` Jeff King
2019-12-17  1:27         ` Emily Shaffer
2019-12-17  1:40           ` Jonathan Nieder [this message]
2019-12-17  5:22           ` Jeff King
2019-12-17  1:43         ` Jonathan Nieder
2019-12-17  5:28           ` Jeff King
2019-12-17 11:35             ` vcxproj target, was " Johannes Schindelin
2019-12-19  4:51               ` Jeff King

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20191217014057.GE10734@google.com \
    --to=jrnieder@gmail$(echo .)com \
    --cc=emilyshaffer@google$(echo .)com \
    --cc=git@vger$(echo .)kernel.org \
    --cc=gitster@pobox$(echo .)com \
    --cc=peff@peff$(echo .)net \
    --cc=ramsay@ramsayjones$(echo .)plus.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox