David Aguilar [off-list ref] 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.
On Tue, Sep 02, 2014 at 11:32:02AM -0700, Junio C Hamano wrote:
David Aguilar [off-list ref] writes:
quoted
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