[PATCH] Documentation/CodingGuidelines: improve header includes rules

[PATCH] Documentation/CodingGuidelines: improve header includes rules

[Christian Couder]

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 Documentation/CodingGuidelines |   26 ++++++++++++++++++++++----
 1 files changed, 22 insertions(+), 4 deletions(-)

	Sorry for the delay but I have been busy with other things.

	The goal of these rule is to codify the current practice and
	to keep things simple to use for the developer while keeping
	some sanity.

	So there are 4 rules and I hope it's enough:

	- The first one was already in the document but is strenghtened
	a bit.

	- The second one existed informaly.

	- The third one means that for example if we have "revision.h"
	that includes "diff.h" and "commit.h", then it's ok to include
	"revision.h" in a C file, only if at least one feature from
	"revision.h" is actually used in the C file.

	It is not ok to include "revision.h" if features from "diff.h"
	and "commit.h" are used but no feature from "revision.h" is
	used.

	But on the other hand if features from bith "revision.h" and
	"diff.h" are used in a C file, then the rule does not state that
	"diff.h" has to be included if "revision.h" is included. So in
	practice this means that it would be ok to remove an include of
	"diff.h" if "revision.h" is included.

	- The fourth and last rule tries to keep things simple to use
	for the developper and quite sane. Here are some part of an email
	I already sent. This will hopefully explain why I think this rule
	is needed:

	> 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 think [the fact that the above fails] 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.

diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
index b8bf618..181cb2d 100644
--- a/Documentation/CodingGuidelines
+++ b/Documentation/CodingGuidelines
@@ -115,10 +115,6 @@ For C programs:
 
  - When you come up with an API, document it.
 
- - The first #include in C files, except in platform specific
-   compat/ implementations, should be git-compat-util.h or another
-   header file that includes it, such as cache.h or builtin.h.
-
  - If you are planning a new command, consider writing it in shell
    or perl first, so that changes in semantics can be easily
    changed and discussed.  Many git commands started out like
@@ -132,3 +128,25 @@ For C programs:
 
  - When we pass <string, length> pair to functions, we should try to
    pass them in that order.
+
+About header file includes:
+
+ - The first #include in C files, except in platform specific
+   "compat/" implementations, should be "git-compat-util.h" or
+   "cache.h" or "builtin.h".
+
+ - System header files should be included in "git-compat-util.h",
+   except for specific system headers like "syslog.h" in "daemon.c".
+   (This is because for portability reasons we want the compiler to
+   always see the same system headers in the same order.)
+
+ - After the first #include in a C file, only header files containing
+   features that are actually used in the C file should be included.
+   (This means that it is not ok to include an header file only
+   because this header file includes other header files with features
+   that are used in the C file.)
+
+ - Git header files, when they are included in a C file after
+   "cache.h", should not require any other header file to be included
+   before them for the C file to compile cleanly. (This is because we
+   want to keep it simple to use features defined in our headers.)
-- 
1.6.2.2.572.g4420a

Re: [PATCH] Documentation/CodingGuidelines: improve header includes rules

[Johannes Schindelin]

Hi,

On Tue, 14 Apr 2009, Christian Couder wrote:

> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---
>  Documentation/CodingGuidelines |   26 ++++++++++++++++++++++----
>  1 files changed, 22 insertions(+), 4 deletions(-)

I think that there is a very real possiblity here to make this document so 
long that hardly anybody reads it to the end.

And is there not a real chance that your change is actually covered by

	As for more concrete guidelines, just imitate the existing code

?

Ciao,
Dscho

Re: [PATCH] Documentation/CodingGuidelines: improve header includes rules

[Jeff King]

On Tue, Apr 14, 2009 at 12:34:33AM +0200, Christian Couder wrote:

> 	- The third one means that for example if we have "revision.h"
> 	that includes "diff.h" and "commit.h", then it's ok to include
> 	"revision.h" in a C file, only if at least one feature from
> 	"revision.h" is actually used in the C file.
> 
> 	It is not ok to include "revision.h" if features from "diff.h"
> 	and "commit.h" are used but no feature from "revision.h" is
> 	used.

Why? I thought the guiding principle mentioned earlier was "don't waste
programmers' time figuring out what should and shouldn't be included".

Sure, I would not expect somebody to include a header that is totally
unrelated, but it seems that most of the source files lazily include
cache.h just to get "everything".

Stripping unnecessary includes doesn't even speed up compilation, as our
Makefile overspecifies the header dependencies anyway.

-Peff

Re: [PATCH] Documentation/CodingGuidelines: improve header includes rules

[Christian Couder]

Le mercredi 15 avril 2009, Jeff King a écrit :
> On Tue, Apr 14, 2009 at 12:34:33AM +0200, Christian Couder wrote:
> > 	- The third one means that for example if we have "revision.h"
> > 	that includes "diff.h" and "commit.h", then it's ok to include
> > 	"revision.h" in a C file, only if at least one feature from
> > 	"revision.h" is actually used in the C file.
> >
> > 	It is not ok to include "revision.h" if features from "diff.h"
> > 	and "commit.h" are used but no feature from "revision.h" is
> > 	used.
>
> Why? I thought the guiding principle mentioned earlier was "don't waste
> programmers' time figuring out what should and shouldn't be included".
>
> Sure, I would not expect somebody to include a header that is totally
> unrelated, but it seems that most of the source files lazily include
> cache.h just to get "everything".

The third rule is:

+ - After the first #include in a C file, only header files containing
+   features that are actually used in the C file should be included.
+   (This means that it is not ok to include an header file only
+   because this header file includes other header files with features
+   that are used in the C file.)

So this rule is only for #include after the first one, and "cache.h" should 
be the first one if it is included.

I added this rule because we need a little sanity too and it can be 
misleading to see an include of some feature when in fact no features from 
the include are used.

> Stripping unnecessary includes doesn't even speed up compilation, as our
> Makefile overspecifies the header dependencies anyway.

Perhaps the dependencies will not be overspecified one day, and I think it 
should at least speed up a little bit compilation of the C file where 
unnecessary includes have been stripped. And anyway it looks simpler and 
does not mislead into thinking that some feature are used when they are 
not.

Best regards,
Christian.