{"thread":{"id":"18857","subject":"[PATCH] Documentation/CodingGuidelines: improve header includes rules","startedAt":"2009-04-13T22:34:33Z","lastAt":"2009-04-16T05:10:28Z","messageCount":4,"participants":["Christian Couder","Johannes Schindelin","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"111245","messageId":"20090414003433.39cbdea2.chriscool@tuxfamily.org","threadId":"18857","inReplyTo":null,"subject":"[PATCH] Documentation/CodingGuidelines: improve header includes rules","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2009-04-13T22:34:33Z","receivedAt":"2009-04-13T22:34:33Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Signed-off-by: Christian Couder <chriscool@tuxfamily.org>\n---\n Documentation/CodingGuidelines |   26 ++++++++++++++++++++++----\n 1 files changed, 22 insertions(+), 4 deletions(-)\n\n\tSorry for the delay but I have been busy with other things.\n\n\tThe goal of these rule is to codify the current practice and\n\tto keep things simple to use for the developer while keeping\n\tsome sanity.\n\n\tSo there are 4 rules and I hope it's enough:\n\n\t- The first one was already in the document but is strenghtened\n\ta bit.\n\n\t- The second one existed informaly.\n\n\t- The third one means that for example if we have \"revision.h\"\n\tthat includes \"diff.h\" and \"commit.h\", then it's ok to include\n\t\"revision.h\" in a C file, only if at least one feature from\n\t\"revision.h\" is actually used in the C file.\n\n\tIt is not ok to include \"revision.h\" if features from \"diff.h\"\n\tand \"commit.h\" are used but no feature from \"revision.h\" is\n\tused.\n\n\tBut on the other hand if features from bith \"revision.h\" and\n\t\"diff.h\" are used in a C file, then the rule does not state that\n\t\"diff.h\" has to be included if \"revision.h\" is included. So in\n\tpractice this means that it would be ok to remove an include of\n\t\"diff.h\" if \"revision.h\" is included.\n\n\t- The fourth and last rule tries to keep things simple to use\n\tfor the developper and quite sane. Here are some part of an email\n\tI already sent. This will hopefully explain why I think this rule\n\tis needed:\n\n\t> Well, I wanted to say that for a header file frotz.h in the\n\t> project:\n\t>\n        > $ cat >1.c <<\\EOF\n        > #include \"cache.h\"\n        > #include \"frotz.h\"\n        > EOF\n        > $ cc -Wall -c 1.c\n        >\n\t> should not fail.\n\t>\n\t> (Ok, in practice, let's say that something like:\n\t>\n\t> $ cc -Wall -DSHA1_HEADER='<openssl/sha.h>' -c 1.c\n\t>\n\t> should not fail.)\n\t>\n\t> I think [the fact that the above fails] may become a problem if\n\t> a header needs more headers to be included before it, and those\n\t> latter headers again need more headers and so on.\n\t>\n\t> The advantage of having and using \"cache.h\" is that things are\n\t> quite simple. \n\t> You include cache.h and then a few more headers for special\n\t> features you need, and, here you go, you can code up something\n\t> quite interesting without worrying too much about includes.\n\t> (Though the compilation time is perhaps a little longer than it\n\t> could be.)\n\t>\n\t> If we loose this simplicity because we don't take care or for\n\t> some theoretical reason, then I think we have lost everything\n\t> that really matters.\n\ndiff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines\nindex b8bf618..181cb2d 100644\n--- a/Documentation/CodingGuidelines\n+++ b/Documentation/CodingGuidelines\n@@ -115,10 +115,6 @@ For C programs:\n \n  - When you come up with an API, document it.\n \n- - The first #include in C files, except in platform specific\n-   compat/ implementations, should be git-compat-util.h or another\n-   header file that includes it, such as cache.h or builtin.h.\n-\n  - If you are planning a new command, consider writing it in shell\n    or perl first, so that changes in semantics can be easily\n    changed and discussed.  Many git commands started out like\n@@ -132,3 +128,25 @@ For C programs:\n \n  - When we pass <string, length> pair to functions, we should try to\n    pass them in that order.\n+\n+About header file includes:\n+\n+ - The first #include in C files, except in platform specific\n+   \"compat/\" implementations, should be \"git-compat-util.h\" or\n+   \"cache.h\" or \"builtin.h\".\n+\n+ - System header files should be included in \"git-compat-util.h\",\n+   except for specific system headers like \"syslog.h\" in \"daemon.c\".\n+   (This is because for portability reasons we want the compiler to\n+   always see the same system headers in the same order.)\n+\n+ - After the first #include in a C file, only header files containing\n+   features that are actually used in the C file should be included.\n+   (This means that it is not ok to include an header file only\n+   because this header file includes other header files with features\n+   that are used in the C file.)\n+\n+ - Git header files, when they are included in a C file after\n+   \"cache.h\", should not require any other header file to be included\n+   before them for the C file to compile cleanly. (This is because we\n+   want to keep it simple to use features defined in our headers.)\n-- \n1.6.2.2.572.g4420a\n"},{"id":"111292","messageId":"alpine.DEB.1.00.0904141822030.10279@pacific.mpi-cbg.de","threadId":"18857","inReplyTo":"20090414003433.39cbdea2.chriscool@tuxfamily.org","subject":"Re: [PATCH] Documentation/CodingGuidelines: improve header includes rules","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-04-14T16:24:43Z","receivedAt":"2009-04-14T16:24:43Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Tue, 14 Apr 2009, Christian Couder wrote:\n\n> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>\n> ---\n>  Documentation/CodingGuidelines |   26 ++++++++++++++++++++++----\n>  1 files changed, 22 insertions(+), 4 deletions(-)\n\nI think that there is a very real possiblity here to make this document so \nlong that hardly anybody reads it to the end.\n\nAnd is there not a real chance that your change is actually covered by\n\n\tAs for more concrete guidelines, just imitate the existing code\n\n?\n\nCiao,\nDscho\n"},{"id":"111343","messageId":"20090415075819.GC23332@coredump.intra.peff.net","threadId":"18857","inReplyTo":"20090414003433.39cbdea2.chriscool@tuxfamily.org","subject":"Re: [PATCH] Documentation/CodingGuidelines: improve header includes rules","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-04-15T07:58:19Z","receivedAt":"2009-04-15T07:58:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 14, 2009 at 12:34:33AM +0200, Christian Couder wrote:\n\n> \t- The third one means that for example if we have \"revision.h\"\n> \tthat includes \"diff.h\" and \"commit.h\", then it's ok to include\n> \t\"revision.h\" in a C file, only if at least one feature from\n> \t\"revision.h\" is actually used in the C file.\n> \n> \tIt is not ok to include \"revision.h\" if features from \"diff.h\"\n> \tand \"commit.h\" are used but no feature from \"revision.h\" is\n> \tused.\n\nWhy? I thought the guiding principle mentioned earlier was \"don't waste\nprogrammers' time figuring out what should and shouldn't be included\".\n\nSure, I would not expect somebody to include a header that is totally\nunrelated, but it seems that most of the source files lazily include\ncache.h just to get \"everything\".\n\nStripping unnecessary includes doesn't even speed up compilation, as our\nMakefile overspecifies the header dependencies anyway.\n\n-Peff\n"},{"id":"111416","messageId":"200904160710.28473.chriscool@tuxfamily.org","threadId":"18857","inReplyTo":"20090415075819.GC23332@coredump.intra.peff.net","subject":"Re: [PATCH] Documentation/CodingGuidelines: improve header includes rules","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2009-04-16T05:10:28Z","receivedAt":"2009-04-16T05:10:28Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Le mercredi 15 avril 2009, Jeff King a écrit :\n> On Tue, Apr 14, 2009 at 12:34:33AM +0200, Christian Couder wrote:\n> > \t- The third one means that for example if we have \"revision.h\"\n> > \tthat includes \"diff.h\" and \"commit.h\", then it's ok to include\n> > \t\"revision.h\" in a C file, only if at least one feature from\n> > \t\"revision.h\" is actually used in the C file.\n> >\n> > \tIt is not ok to include \"revision.h\" if features from \"diff.h\"\n> > \tand \"commit.h\" are used but no feature from \"revision.h\" is\n> > \tused.\n>\n> Why? I thought the guiding principle mentioned earlier was \"don't waste\n> programmers' time figuring out what should and shouldn't be included\".\n>\n> Sure, I would not expect somebody to include a header that is totally\n> unrelated, but it seems that most of the source files lazily include\n> cache.h just to get \"everything\".\n\nThe third rule is:\n\n+ - After the first #include in a C file, only header files containing\n+   features that are actually used in the C file should be included.\n+   (This means that it is not ok to include an header file only\n+   because this header file includes other header files with features\n+   that are used in the C file.)\n\nSo this rule is only for #include after the first one, and \"cache.h\" should \nbe the first one if it is included.\n\nI added this rule because we need a little sanity too and it can be \nmisleading to see an include of some feature when in fact no features from \nthe include are used.\n\n> Stripping unnecessary includes doesn't even speed up compilation, as our\n> Makefile overspecifies the header dependencies anyway.\n\nPerhaps the dependencies will not be overspecified one day, and I think it \nshould at least speed up a little bit compilation of the C file where \nunnecessary includes have been stripped. And anyway it looks simpler and \ndoes not mislead into thinking that some feature are used when they are \nnot.\n\nBest regards,\nChristian.\n"}]}