Re: [RFC PATCH 3/3] core: improve header dependencies

From: David Aguilar <davvid@gmail.com>
Date: Sep 2, 2014, 19:19 UTC
Message-ID: <20140902191901.GA78128@gmail.com>
In-Reply-To: <xmqqr3zt27rx.fsf@gitster.dls.corp.google.com>

On Tue, Sep 02, 2014 at 11:32:02AM -0700, Junio C Hamano wrote:

Show 37 quoted lines

> David Aguilar <davvid@gmail.com> writes:
> 
> > Remove includes that have already been included by another header.
> 
> Hmm, I am not sure if that is a good move, and suspect that it is
> incompatible with what your 2/3 attempts to do, at least at the
> philosophical level.
> 
> I am guessing that your 2/3 wants to see
> 
> 	gcc $header.h
> 
> to be happy.  One benefit from doing such a change is that sources
> that want to use declaration made in $header.h have to include that
> $header.h without having to worry about what other things the
> implementation detail of $header.h needs.  If function F or type T
> is declared in header H, you include H and you are done.
> 
> That is nice and tidy, but if that is the goal, then after making H
> include its own dependency H1 that happen to declare functions F1,
> F2 and types T1, T2 (which are necessary for H to be complete as
> standalone), if the source that used to include both H and H1
> because it uses F and F1 should still explicitly include H1, no?
> 
> For example, you dropped "diff.h" from builtin/add.c, but the
> implementation of builtin/add.c needs access to diff_options struct,
> which is in "diff.h", not whatever happened to include indirectly
> that is already included by builtin/add.c.  I do not think it is a
> good idea, and more importantly I suspect that it is not consistent
> with what you tried to do with your 2/3.
> 
> But it is entirely possible I am misunderstanding the real
> motivation behind these changes.  The log message justifies why
> removal is safe i.e. "have already been included indirectly", and
> the title claims it is an improvement, but there is no explanation
> why it is an improvement (which would have also explained the
> motivation behind it), so it is a bit hard for me to guess.

The commit messages can certainly be improved.

Patch 2/3 is really a question in patch form:

Should we (a) forward-declare structs in headers that use pointers to those structs, or (b) full-on #include the headers that define those structs?

Making "gcc $header.h" happy might be good but that wasn't the original motivation; approach (b) would satisfy that.

This old (and maybe uneditable these days?) wiki page contains items that were the motivation for these patches:

https://git.wiki.kernel.org/index.php/Janitor

- Some Git headers depend on other headers to compile cleanly,
  in this case it might be a good idea to include the needed
  headers in the header that needs them."

- For example "revision.h" depends on "commit.h" and "diff.h",
  so "revision.h" should include them.

- Of course after that it makes sense to clean up the "*.c"
  source files that include all these headers, to remove the
  headers that are no more needed there.

Most importantly, though, is this part:

- Contact the mail list to see how to proceed.

;-)

Patch 3/3 is the item about cleaning up *.c files, but we may want to reformulate these items and take a different approach.

Thoughts?

-- 
David