Re: [PATCH 0/5] Header includes cleanup

[Christian Couder <chriscool@tuxfamily.org>]

Le jeudi 2 avril 2009, Junio C Hamano a écrit :
> Christian Couder <chriscool@tuxfamily.org> writes:
> > Ok, so I suggest the following simple guiding principles:
> >
> > - git-compat-util.h or cache.h or builtin.h should always be the first
> > #include in C files,
> >
> > - header files should include other incude files if and only if the
> > other includes are needed to compile them,
>
> This is unclear.
>
> Long before I started touching git, I used to be religious about making
> header files self contained.  For a header file frotz.h in the project, I
> used to insist that
>
> 	$ cat >1.c <<\EOF
> 	#include "frotz.h"
>         EOF
>         $ cc -Wall -c 1.c
>
> did not fail.
>
> Are you talking about that by "to compile them"?

Well, I wanted to say that for a header file frotz.h in the project:

	$ cat >1.c <<\EOF
	#include "cache.h"
	#include "frotz.h"
	EOF
	$ cc -Wall -c 1.c
	
should not fail.

(Ok, in practice, let's say that something like:

$ cc -Wall -DSHA1_HEADER='<openssl/sha.h>' -c 1.c

should not fail.)

> I stopped doing that long time ago, partly because the rule was
> cumbersome to enforce, but primarily because it was not helping much in
> the larger picture.
>
> Such a rule, together with strict rules such as the order of including
> other header files in the header files themselves, may make life easier
> for programmers who touch .c files but never .h files, because they can
> include only the necessary .h files and in any order.  But in practice,
> people need to touch both .h and .c files and when they need to include
> new system header files, they need to follow the inclusion order rule
> somewhere anyway---at that point, it does not matter much if the rule
> applies to only .h files or both .h and .c files.
>
> So for example, you cannot compile
>
> 	$ cat >1.c <<\EOF
>         #include "revision.h"
>         EOF
>         $ cc -Wll -c 1.c
>
> in git.git project, but I do not think it is a problem.

I think it may become a problem if a header needs more headers to be 
included before it, and those latter headers again need more headers and so 
on.

The advantage of having and using "cache.h" is that things are quite simple. 
You include cache.h and then a few more headers for special features you 
need, and, here you go, you can code up something quite interesting without 
worrying too much about includes. (Though the compilation time is perhaps a 
little longer than it could be.)

If we loose this simplicity because we don't take care or for some 
theoretical reason, then I think we have lost everything that really 
matters.

A simple patch like this:

---------8<------------
$ gdiff
diff --git a/revision.h b/revision.h
index 5adfc91..495d7eb 100644
--- a/revision.h
+++ b/revision.h
@@ -3,6 +3,8 @@

 #include "parse-options.h"
 #include "grep.h"
+#include "diff.h"
+#include "commit.h"

 #define SEEN           (1u<<0)
 #define UNINTERESTING   (1u<<1)
---------8<------------

makes the following just work(tm):

	$ cat >1.c <<\EOF
	#include "cache.h"
	#include "revision.h"
	EOF
	$ cc -Wall -DSHA1_HEADER='<openssl/sha.h>' -c 1.c
	$

(By the way it's this kind of patches that I hoped would get posted first.)

> > - a header file should be included in a C file only if it is needed to
> > compile the C file (it is not ok to include it only because it includes
> > many other headers that are needed)

I should have said "a header file other than git-compat-util.h or cache.h 
should be included ..."

> If that is the rule, perhaps the problem lies not in a .c program that
> includes such a .h header, but in the .h itself that includes many other
> header files.

It depends if the .h that includes many other header files does that for a 
good reason or not.

> > - other than the above rules, it is ok to reduce the number of includes
> > as much as possible
> >
> > What do you think?
>
> What you did not write and I forgot to mention, which is a logical
> conclusion of the first rule, is that C files should not directly include
> common system header files such as unistd, sys/stat, etc.

Yeah, I forgot to restate that.

> There are exceptions to any rule.  For example, inclusion of syslog.h in
> daemon.c is OK because most of the rest of the system does not even use
> syslog.  If we later find a platform whose syslog.h has some funny
> inter-header dependencies, however, we will need to include it in the
> git-compat-util.h and resolve the dependencies there, like we do for
> other system header files.

Ok, I will state that too.

If you agree I will post a patch to "Documentation/CodingGuidelines" with 
the above clarifications. (I can also post an improved version of my patch 
to "revision.h".)

Thanks,
Christian.

> > Or perhaps Junio would prefer that you work on a C file by C file
> > basis? Like for example:
> >
> > "delete useless includes in 'builtin-diff-files.c'"
> > "delete useless includes in 'builtin-diff-index.c'"
>
> If the series does not involve .h file clean-up, then a series that
> consists of one patch per .c file would be easier to handle, I think.