| Message ID | xmqqle76kdpr.fsf_-_@gitster.g (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 4e89f0e07cfba1155c012728e14f8362ab6a167f |
| Headers | show |
| Series | [v3] doc: clarify the wording on <git-compat-util.h> requirement | expand |
On Mon, Feb 26, 2024 at 3:28 PM Junio C Hamano <gitster@pobox.com> wrote: > > The reason why we require the <git-compat-util.h> file to be the > first header file to be included is because it insulates other > header files and source files from platform differences, like which > system header files must be included in what order, and what C > preprocessor feature macros must be defined to trigger certain > features we want out of the system. > > We tried to clarify the rule in the coding guidelines document, but > the wording was a bit fuzzy that can lead to misinterpretations like > you can include <xdiff/xinclude.h> only to avoid having to include > <git-compat-util.h> even if you have nothing to do with the xdiff > implementation, for example. "You do not have to include more than > one of these" was also misleading and would have been puzzling if > you _needed_ to depend on more than one of these approved headers > (answer: you are allowed to include them all if you need the > declarations in them for reasons other than that you want to avoid > including compat-util yourself). > > Instead of using the phrase "approved headers", enumerate them as > exceptions, each labeled with its intended audiences, to avoid such > misinterpretations. The structure also makes it easier to add new > exceptions, so add the description of "t/unit-tests/test-lib.h" > being an exception only for the unit tests implementation as an > example. > > Signed-off-by: Junio C Hamano <gitster@pobox.com> > --- > > * Updated the leading phrase introducing the list of exceptions. > I think this is now clear enough to be ready for 'next'? > > Range-diff: > 1: 2e7082d2d2 ! 1: 470f33a078 doc: clarify the wording on <git-compat-util.h> requirement > @@ Documentation/CodingGuidelines: For C programs: > - "t/helper/test-tool.h", "xdiff/xinclude.h", or > - "reftable/system.h".) You do not have to include more than one of > - these. > -+ implementations and sha1dc/, must be "git-compat-util.h". This > ++ implementations and sha1dc/, must be <git-compat-util.h>. This > + header file insulates other header files and source files from > + platform differences, like which system header files must be > + included in what order, and what C preprocessor feature macros must > + be defined to trigger certain features we expect out of the system. > ++ A collorary to this is that C files should not directly include > ++ system header files themselves. > + > -+ In addition: > ++ There are some exceptions, because certain group of files that > ++ implement an API all have to include the same header file that > ++ defines the API and it is convenient to include <git-compat-util.h> > ++ there. Namely: > + > + - the implementation of the built-in commands in the "builtin/" > + directory that include "builtin.h" for the cmd_foo() prototype > -+ definition > ++ definition, > + > + - the test helper programs in the "t/helper/" directory that include > -+ "t/helper/test-tool.h" for the cmd__foo() prototype definition > ++ "t/helper/test-tool.h" for the cmd__foo() prototype definition, > + > + - the xdiff implementation in the "xdiff/" directory that includes > -+ "xdiff/xinclude.h" for the xdiff machinery internals > ++ "xdiff/xinclude.h" for the xdiff machinery internals, > + > + - the unit test programs in "t/unit-tests/" directory that include > + "t/unit-tests/test-lib.h" that gives them the unit-tests > -+ framework > ++ framework, and > + > + - the source files that implement reftable in the "reftable/" > + directory that include "reftable/system.h" for the reftable > -+ internals > ++ internals, > + > + are allowed to assume that they do not have to include > -+ "git-compat-util.h" themselves, as it is included as the first > ++ <git-compat-util.h> themselves, as it is included as the first > + '#include' in these header files. These headers must be the first > + header file to be "#include"d in them, though. > > > Documentation/CodingGuidelines | 41 +++++++++++++++++++++++++++++----- > 1 file changed, 35 insertions(+), 6 deletions(-) > > diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines > index 578587a471..806979f75b 100644 > --- a/Documentation/CodingGuidelines > +++ b/Documentation/CodingGuidelines > @@ -446,12 +446,41 @@ For C programs: > detail. > > - The first #include in C files, except in platform specific compat/ > - implementations and sha1dc/, must be either "git-compat-util.h" or > - one of the approved headers that includes it first for you. (The > - approved headers currently include "builtin.h", > - "t/helper/test-tool.h", "xdiff/xinclude.h", or > - "reftable/system.h".) You do not have to include more than one of > - these. > + implementations and sha1dc/, must be <git-compat-util.h>. This > + header file insulates other header files and source files from > + platform differences, like which system header files must be > + included in what order, and what C preprocessor feature macros must > + be defined to trigger certain features we expect out of the system. > + A collorary to this is that C files should not directly include > + system header files themselves. > + > + There are some exceptions, because certain group of files that > + implement an API all have to include the same header file that > + defines the API and it is convenient to include <git-compat-util.h> > + there. Namely: > + > + - the implementation of the built-in commands in the "builtin/" > + directory that include "builtin.h" for the cmd_foo() prototype > + definition, > + > + - the test helper programs in the "t/helper/" directory that include > + "t/helper/test-tool.h" for the cmd__foo() prototype definition, > + > + - the xdiff implementation in the "xdiff/" directory that includes > + "xdiff/xinclude.h" for the xdiff machinery internals, > + > + - the unit test programs in "t/unit-tests/" directory that include > + "t/unit-tests/test-lib.h" that gives them the unit-tests > + framework, and > + > + - the source files that implement reftable in the "reftable/" > + directory that include "reftable/system.h" for the reftable > + internals, > + > + are allowed to assume that they do not have to include > + <git-compat-util.h> themselves, as it is included as the first > + '#include' in these header files. These headers must be the first > + header file to be "#include"d in them, though. > > - A C file must directly include the header files that declare the > functions and the types it uses, except for the functions and types > -- > 2.44.0 > Looks good
On Mon, Feb 26, 2024 at 3:28 PM Junio C Hamano <gitster@pobox.com> wrote: > > The reason why we require the <git-compat-util.h> file to be the > first header file to be included is because it insulates other > header files and source files from platform differences, like which > system header files must be included in what order, and what C > preprocessor feature macros must be defined to trigger certain > features we want out of the system. > > We tried to clarify the rule in the coding guidelines document, but > the wording was a bit fuzzy that can lead to misinterpretations like > you can include <xdiff/xinclude.h> only to avoid having to include > <git-compat-util.h> even if you have nothing to do with the xdiff > implementation, for example. "You do not have to include more than > one of these" was also misleading and would have been puzzling if > you _needed_ to depend on more than one of these approved headers > (answer: you are allowed to include them all if you need the > declarations in them for reasons other than that you want to avoid > including compat-util yourself). > > Instead of using the phrase "approved headers", enumerate them as > exceptions, each labeled with its intended audiences, to avoid such > misinterpretations. The structure also makes it easier to add new > exceptions, so add the description of "t/unit-tests/test-lib.h" > being an exception only for the unit tests implementation as an > example. Makes sense. > Signed-off-by: Junio C Hamano <gitster@pobox.com> > --- > > * Updated the leading phrase introducing the list of exceptions. > I think this is now clear enough to be ready for 'next'? > > Range-diff: > 1: 2e7082d2d2 ! 1: 470f33a078 doc: clarify the wording on <git-compat-util.h> requirement > @@ Documentation/CodingGuidelines: For C programs: > - "t/helper/test-tool.h", "xdiff/xinclude.h", or > - "reftable/system.h".) You do not have to include more than one of > - these. > -+ implementations and sha1dc/, must be "git-compat-util.h". This > ++ implementations and sha1dc/, must be <git-compat-util.h>. This > + header file insulates other header files and source files from > + platform differences, like which system header files must be > + included in what order, and what C preprocessor feature macros must > + be defined to trigger certain features we expect out of the system. > ++ A collorary to this is that C files should not directly include > ++ system header files themselves. > + > -+ In addition: > ++ There are some exceptions, because certain group of files that > ++ implement an API all have to include the same header file that > ++ defines the API and it is convenient to include <git-compat-util.h> > ++ there. Namely: > + > + - the implementation of the built-in commands in the "builtin/" > + directory that include "builtin.h" for the cmd_foo() prototype > -+ definition > ++ definition, > + > + - the test helper programs in the "t/helper/" directory that include > -+ "t/helper/test-tool.h" for the cmd__foo() prototype definition > ++ "t/helper/test-tool.h" for the cmd__foo() prototype definition, > + > + - the xdiff implementation in the "xdiff/" directory that includes > -+ "xdiff/xinclude.h" for the xdiff machinery internals > ++ "xdiff/xinclude.h" for the xdiff machinery internals, > + > + - the unit test programs in "t/unit-tests/" directory that include > + "t/unit-tests/test-lib.h" that gives them the unit-tests > -+ framework > ++ framework, and > + > + - the source files that implement reftable in the "reftable/" > + directory that include "reftable/system.h" for the reftable > -+ internals > ++ internals, > + > + are allowed to assume that they do not have to include > -+ "git-compat-util.h" themselves, as it is included as the first > ++ <git-compat-util.h> themselves, as it is included as the first > + '#include' in these header files. These headers must be the first > + header file to be "#include"d in them, though. > > > Documentation/CodingGuidelines | 41 +++++++++++++++++++++++++++++----- > 1 file changed, 35 insertions(+), 6 deletions(-) > > diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines > index 578587a471..806979f75b 100644 > --- a/Documentation/CodingGuidelines > +++ b/Documentation/CodingGuidelines > @@ -446,12 +446,41 @@ For C programs: > detail. > > - The first #include in C files, except in platform specific compat/ > - implementations and sha1dc/, must be either "git-compat-util.h" or > - one of the approved headers that includes it first for you. (The > - approved headers currently include "builtin.h", > - "t/helper/test-tool.h", "xdiff/xinclude.h", or > - "reftable/system.h".) You do not have to include more than one of > - these. > + implementations and sha1dc/, must be <git-compat-util.h>. This > + header file insulates other header files and source files from > + platform differences, like which system header files must be > + included in what order, and what C preprocessor feature macros must > + be defined to trigger certain features we expect out of the system. > + A collorary to this is that C files should not directly include > + system header files themselves. > + > + There are some exceptions, because certain group of files that > + implement an API all have to include the same header file that > + defines the API and it is convenient to include <git-compat-util.h> > + there. Namely: > + > + - the implementation of the built-in commands in the "builtin/" > + directory that include "builtin.h" for the cmd_foo() prototype > + definition, > + > + - the test helper programs in the "t/helper/" directory that include > + "t/helper/test-tool.h" for the cmd__foo() prototype definition, > + > + - the xdiff implementation in the "xdiff/" directory that includes > + "xdiff/xinclude.h" for the xdiff machinery internals, > + > + - the unit test programs in "t/unit-tests/" directory that include > + "t/unit-tests/test-lib.h" that gives them the unit-tests > + framework, and > + > + - the source files that implement reftable in the "reftable/" > + directory that include "reftable/system.h" for the reftable > + internals, > + > + are allowed to assume that they do not have to include > + <git-compat-util.h> themselves, as it is included as the first > + '#include' in these header files. These headers must be the first > + header file to be "#include"d in them, though. > > - A C file must directly include the header files that declare the > functions and the types it uses, except for the functions and types > -- > 2.44.0 I like this latest version.
Elijah Newren <newren@gmail.com> writes:
> I like this latest version.
Thanks.
diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines index 578587a471..806979f75b 100644 --- a/Documentation/CodingGuidelines +++ b/Documentation/CodingGuidelines @@ -446,12 +446,41 @@ For C programs: detail. - The first #include in C files, except in platform specific compat/ - implementations and sha1dc/, must be either "git-compat-util.h" or - one of the approved headers that includes it first for you. (The - approved headers currently include "builtin.h", - "t/helper/test-tool.h", "xdiff/xinclude.h", or - "reftable/system.h".) You do not have to include more than one of - these. + implementations and sha1dc/, must be <git-compat-util.h>. This + header file insulates other header files and source files from + platform differences, like which system header files must be + included in what order, and what C preprocessor feature macros must + be defined to trigger certain features we expect out of the system. + A collorary to this is that C files should not directly include + system header files themselves. + + There are some exceptions, because certain group of files that + implement an API all have to include the same header file that + defines the API and it is convenient to include <git-compat-util.h> + there. Namely: + + - the implementation of the built-in commands in the "builtin/" + directory that include "builtin.h" for the cmd_foo() prototype + definition, + + - the test helper programs in the "t/helper/" directory that include + "t/helper/test-tool.h" for the cmd__foo() prototype definition, + + - the xdiff implementation in the "xdiff/" directory that includes + "xdiff/xinclude.h" for the xdiff machinery internals, + + - the unit test programs in "t/unit-tests/" directory that include + "t/unit-tests/test-lib.h" that gives them the unit-tests + framework, and + + - the source files that implement reftable in the "reftable/" + directory that include "reftable/system.h" for the reftable + internals, + + are allowed to assume that they do not have to include + <git-compat-util.h> themselves, as it is included as the first + '#include' in these header files. These headers must be the first + header file to be "#include"d in them, though. - A C file must directly include the header files that declare the functions and the types it uses, except for the functions and types
The reason why we require the <git-compat-util.h> file to be the first header file to be included is because it insulates other header files and source files from platform differences, like which system header files must be included in what order, and what C preprocessor feature macros must be defined to trigger certain features we want out of the system. We tried to clarify the rule in the coding guidelines document, but the wording was a bit fuzzy that can lead to misinterpretations like you can include <xdiff/xinclude.h> only to avoid having to include <git-compat-util.h> even if you have nothing to do with the xdiff implementation, for example. "You do not have to include more than one of these" was also misleading and would have been puzzling if you _needed_ to depend on more than one of these approved headers (answer: you are allowed to include them all if you need the declarations in them for reasons other than that you want to avoid including compat-util yourself). Instead of using the phrase "approved headers", enumerate them as exceptions, each labeled with its intended audiences, to avoid such misinterpretations. The structure also makes it easier to add new exceptions, so add the description of "t/unit-tests/test-lib.h" being an exception only for the unit tests implementation as an example. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- * Updated the leading phrase introducing the list of exceptions. I think this is now clear enough to be ready for 'next'? Range-diff: 1: 2e7082d2d2 ! 1: 470f33a078 doc: clarify the wording on <git-compat-util.h> requirement @@ Documentation/CodingGuidelines: For C programs: - "t/helper/test-tool.h", "xdiff/xinclude.h", or - "reftable/system.h".) You do not have to include more than one of - these. -+ implementations and sha1dc/, must be "git-compat-util.h". This ++ implementations and sha1dc/, must be <git-compat-util.h>. This + header file insulates other header files and source files from + platform differences, like which system header files must be + included in what order, and what C preprocessor feature macros must + be defined to trigger certain features we expect out of the system. ++ A collorary to this is that C files should not directly include ++ system header files themselves. + -+ In addition: ++ There are some exceptions, because certain group of files that ++ implement an API all have to include the same header file that ++ defines the API and it is convenient to include <git-compat-util.h> ++ there. Namely: + + - the implementation of the built-in commands in the "builtin/" + directory that include "builtin.h" for the cmd_foo() prototype -+ definition ++ definition, + + - the test helper programs in the "t/helper/" directory that include -+ "t/helper/test-tool.h" for the cmd__foo() prototype definition ++ "t/helper/test-tool.h" for the cmd__foo() prototype definition, + + - the xdiff implementation in the "xdiff/" directory that includes -+ "xdiff/xinclude.h" for the xdiff machinery internals ++ "xdiff/xinclude.h" for the xdiff machinery internals, + + - the unit test programs in "t/unit-tests/" directory that include + "t/unit-tests/test-lib.h" that gives them the unit-tests -+ framework ++ framework, and + + - the source files that implement reftable in the "reftable/" + directory that include "reftable/system.h" for the reftable -+ internals ++ internals, + + are allowed to assume that they do not have to include -+ "git-compat-util.h" themselves, as it is included as the first ++ <git-compat-util.h> themselves, as it is included as the first + '#include' in these header files. These headers must be the first + header file to be "#include"d in them, though. Documentation/CodingGuidelines | 41 +++++++++++++++++++++++++++++----- 1 file changed, 35 insertions(+), 6 deletions(-)