Re: [PATCH 0/5] Header includes cleanup

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 2, 2009, 05:25 UTC
Message-ID: <7v8wmjk4p6.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <200904020557.25058.chriscool@tuxfamily.org>

Christian Couder <chriscool@tuxfamily.org> writes:

Show 7 quoted lines

> 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"?

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.

> - 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)

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.

> - 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.

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.

Show 5 quoted lines

> 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.