Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SF.12: Prefer the quoted form of #include for files relative to the… #1596

Merged
merged 9 commits into from
Jul 25, 2020
Merged

SF.12: Prefer the quoted form of #include for files relative to the… #1596

merged 9 commits into from
Jul 25, 2020

Conversation

apenn-msft
Copy link
Contributor

… including file

The current guidance on SF.12 can be over-applied and devolves into "always use <>" because all compilers support adding include directories to the <> search. In this case, even the current directory may be added and so it is always possible to use <> for every header. Applying the guidance then devolves into an undesirable state where <> is always used and include"" is never used.

Instead, the proposed guidance leverages and encourages the distinction between <> and "" to create an easy-to-understand rule that the original guidance hints at and that most developers already follow and understand: "" is for local headers and <> is for library and external headers.

… including file

The current guidance on SF.12 can be over-applied and devolves into "always use <>" because all compilers support adding include directories to the <> search. In this case, even the current directory may be added and so it is always possible to use <> for every header. Applying the guidance then devolves into an undesirable state where <> is always used and include"" is never used.

Instead, the proposed guidance leverages and encourages the distinction between <> and "" to create an easy-to-understand rule that the original guidance hints at and that most developers already follow and understand: "" is for local headers and <> is for library and external headers.
CppCoreGuidelines.md Outdated Show resolved Hide resolved
@apenn-msft apenn-msft marked this pull request as ready for review April 2, 2020 22:57
CppCoreGuidelines.md Outdated Show resolved Hide resolved
CppCoreGuidelines.md Outdated Show resolved Hide resolved
@MikeGitb
Copy link

MikeGitb commented Apr 3, 2020

I like it.

…clude` for files relative

Updated wording, adjusted example, and provided a more verbose example of what can go wrong if using the wrong form.
CppCoreGuidelines.md Outdated Show resolved Hide resolved
CppCoreGuidelines.md Outdated Show resolved Hide resolved
@Quuxplusone
Copy link
Contributor

If you're writing a library, you must use the angle-bracket form:

// inside mine/mine.h
#include <mine/detail.h>

This line will work no matter whether mine/mine.h is located in ~/sourcetree/src/ or in /usr/include — basically, if anybody ever includes this file via #include <mine/mine.h>, then we automatically know that the exact same include-search-path will be able to find <mine/detail.h>. On the other hand, doing #include "detail.h" might or might not work (depending on compiler options that ought to be irrelevant to us as library writers).

@apenn-msft
Copy link
Contributor Author

If you're writing a library, you must use the angle-bracket form:

Correct; I didn't mention it in SF.12 because it's not an exception to the rule: including "mine/detail.h" is not an option. But there should be another guideline that says as a library creator, you should place your headers along a unique path (author/component/header.h) and include your own headers the same way your users will (e.g. prefer <quux/mine/detail.h> vs "detail.h"). Checking the GSL I can't find it such a guidance, so I created an issue for us to go create one:
#1609

@apenn-msft
Copy link
Contributor Author

@Quuxplusone can you describe in detail more what you meant?

I think I misinterpreted it when I created:
#1609

I mistakenly thought using "detail.h" from mine.h would not be found when included from user code, but I'm reminded that the include is relative from the including file (and not just the final cpp file). In this case, as @MikeGitb suggests, I think it's reasonable for library writers to also use the #include "detail.h" form since this will almost always locate the same detail.h as #include <mine/detail.h>.

If we also exactly follow the guideline of this PR for SF.12, then it would also suggest that the latter form is preferred (e.g. detail.h will always be at a local relative path to mine.h, so #include "detail.h" should be preferred)

@Quuxplusone
Copy link
Contributor

Hmm.

Bloomberg BDE guideline 4.3.4: "All include directives must use angle brackets (i.e., '< >'); the quoted form must not be used."

JSF AV Rule 33: "The #include directive shall use the <filename.h> notation to include header files. [...] The include form "filename.h" is typically used to include local header files. However, due to the unfortunate divergence in vendor implementations, only the <filename.h> form will be used." (No further information on what this implementation divergence might be.)

Google C++ style guide doesn't directly address it, but shows a mix of "" and <> essentially matching Facebook's guideline. Interestingly, they do not treat "" paths as relative to the current directory; "base/foo.cc" will do #include "base/foo.h", not #include "foo.h".

Facebook HHVM guidelines: "Use double quotes for Folly and HHVM headers, and angle brackets for all others."

So I guess the trouble with "", if there is one, is illustrated by Google's guideline: they clearly expect that if "myProject/src/base/foo.cpp" contains the line #include "base/foo.h", it will start its search in myProject/include/, whereas @MikeGitb expects it to start its search in myProject/src/base/. These can't both be true for the same project, right?

There may be a distinction to be made (even though none of the guidelines above explicitly make this distinction) between "public-facing headers" and "private implementation headers." For example, if your project contains

myProject/
- src/
- - base/
- - - foo.cpp
- - - foo_internal.h
- include/
- - base/
- - - foo.h
- - - config.h

then it might be reasonable for "src/base/foo.cpp" to contain the lines

#include <base/foo.h>  // never "foo.h" or "base/foo.h"
#include "foo_internal.h"  // or "base/foo_internal.h" in Google terms?

However, I hope we all agree that the public-facing header "foo.h" should never ever contain

#include "../../src/base/foo_internal.h"  // NO!

and if the public-facing header "foo.h" needs to consume one of its public-facing siblings, it should do it via its "public-facing identifier"

#include <base/config.h>  // never "config.h" or "base/config.h"

I believe Bloomberg gets by with the simpler guideline (which is also what I was pushing for) simply because they never write "private implementation headers."

@MikeGitb
Copy link

MikeGitb commented Apr 19, 2020

So I guess the trouble with "", if there is one, is illustrated by Google's guideline: they clearly expect that if "myProject/src/base/foo.cpp" contains the line #include "base/foo.h", it will start its search in myProject/include/, whereas @MikeGitb expects it to start its search in myProject/src/base/. These can't both be true for the same project, right?

It's quite simple: On clang, gcc, msvc and a few other compilers that I don't remember,

 #include "some/path/header.h"

will first look relative to the current directory (the directory in which the file currently being processed resides) and second relative to whatever include paths are built-in to the compiler (/usr/include) or have been specified on the command line (-I/path/to/lib/foo/include).

#include <some/path/header.h>

Will only look relative to the paths built-in or specified on the command line.
Not sure what implementation divergence some of the guidelines mentioned, but of course there are many, many old and embedded compilers that I don't have any experience with.

@brenoguim
Copy link
Contributor

This is surprising. My conclusion was precisely the opposite last time I thought about this.

If the library uses angle brackets, then it relies on compiler configuration (include paths) be passed. After all, the user code could be doing:

#include <root_of_project/third_party/some_other_grouping/library/header.h>

That won't work unless I add root_of_project/third_party/some_other_grouping to the include path as well.

If the library uses quotes (which I believe to mean "relative includes"), then there is no extra configuration needed. As soon as the user code can include the header.h, it will be able to find any other headers it needs from its own installation.

CppCoreGuidelines.md Outdated Show resolved Hide resolved
@johnmcfarlane
Copy link
Contributor

johnmcfarlane commented Apr 22, 2020

@Quuxplusone

However, I hope we all agree that the public-facing header "foo.h" should never ever contain

#include "../../src/base/foo_internal.h" // NO!

I disagree and would like to know what your reasoning is. Consider contributing to the discussion here. (To clarify, I agree that the choice to include -- at all -- this particular file looks highly dubious, but the general belief that ../ should never be used is common and misguided. Can we be clear on that?)

make it more clear that using <> for external projects is just a typical example (and not the only use case for <> outside of standard headers)
CppCoreGuidelines.md Outdated Show resolved Hide resolved
.... if a bit more monotonous, but that's ok.
@dawidpilarski
Copy link

dawidpilarski commented May 6, 2020

Just a note. The c++ stanard draft actually says: use <> for standard library and "" for anything else:

Although an implementation may provide a mechanism for making arbitrary source files available to the < > search, in general programmers should use the < > form for headers provided with the implementation, and the " " form for sources outside the control of the implementation.
For instance:

#include <stdio.h>
#include <unistd.h>
#include "usefullib.h"
#include "myprog.h"

http://eel.is/c++draft/cpp.include#8.note-1

this is a note however, and it might be an outdated one.

@MikeGitb
Copy link

MikeGitb commented May 6, 2020

I think it completely ignores the reality, but it is an interesting point.

@hsutter
Copy link
Contributor

hsutter commented May 7, 2020

Editors call: We agree the existing guideline is somewhat vague, but the quoted form is less portable and there is no consensus at this time for an improvement in the wording. We agree with the feedback mentioned in the discussion thread, including that if writing a library one should prefer the angle form so that you can have a more predictable search algorithm.

@hsutter hsutter closed this May 7, 2020
@hsutter hsutter self-assigned this May 7, 2020
@gdr-at-ms
Copy link
Contributor

gdr-at-ms commented Jul 22, 2020

This is a much better version than the original version that was submitted. However, I still see some issues. When it says "exist at a relative path", is that an invitation to scrutinize the installation location and decide based on that whether to use the quoted form, or is that an invitation to exploit the internal coherence of a component? That is, if my library's header is going to be installed in /usr/include next to gmp.h , should I use "gmp.h"? I think that would be terrible advice.

I agree that terms such as "project" are too ambiguous.

@apenn-msft
Copy link
Contributor Author

is that an invitation to scrutinize the installation location and decide based on that whether to use the quoted form, or is that an invitation to exploit the internal coherence of a component?

the latter but not the former: the library author can't know where the library will be installed or what headers will be co-located with it, outside of the headers it provides.

@gdr-at-ms
Copy link
Contributor

is that an invitation to scrutinize the installation location and decide based on that whether to use the quoted form, or is that an invitation to exploit the internal coherence of a component?

the latter but not the former: the library author can't know where the library will be installed or what headers will be co-located with it, outside of the headers it provides.

Fantastic!

The wording isn't reflecting that though since it talks about "exist at a relative path": once the library header is installed, it will exist at a relative path. So we need to clarify that.

How about "use the quoted form if the header being included is part of the same component therefore is expected to exist with the including header as a coherent whole"?

@apenn-msft
Copy link
Contributor Author

apenn-msft commented Jul 23, 2020

it should be covered under the same rule, but with the implication being "at the current location of authoring the file". I can make that explicit if need be?
e.g. use the "" form for headers that exist at a relative path, to the location of the header being modified.

(but I do feel it's pretty clear that "the header" here refers to the one being authored and not any future copy of itself placed into the compiler install location, since no one could reason about the location or neighbors of such a file or hope to edit it ... except if doing something unorthodox like authoring a header library directly in /usr/include!)

added the following blurb:
"
Note that this guidance applies to the including file as it exists in the location where it is typically authored/edited, and not any other location to which it may subsequently be copied or installed or otherwise made available to the implementation.
"

explicitly note that "file" refers to as it exists in the location of it being authored/modified.
this is to address any confusion about #including "bar.h" from foo.h based on whether bar.h is relative to foo.h at the time and location foo.h is being modified (e.g. under mylib/foo.h) versus at the time/location from which foo.h is installed to the system and included by the user (e.g. from /usr/include).
@gdr-at-ms
Copy link
Contributor

added the following blurb:
"
Note that this guidance applies to the including file as it exists in the location where it is typically authored/edited, and not any other location to which it may subsequently be copied or installed or otherwise made available to the implementation.
"

That makes clearer your intent; thanks! The rule as written is still approaching the issue from an "implementation" perspective ("at it exists in the location where it is typically authored/edited") versus expressing clearly the desired structure. The desired structure is invariable independently of where the header ended up being found by the compiler or a reader -- either during build time of the component, or after installation. The reason I am emphasizing this aspect is because people will copy the structure they see when they are consulting/reading. This is one of the reasons why in my previous message I talked about component. The header file could have been generated on the fly -- that shouldn't matter. What matters is the final structure and what form of includes is used to express intent.

If you can adjust the text to express that, that would be awesome. Thanks!

@apenn-msft
Copy link
Contributor Author

apenn-msft commented Jul 24, 2020

I want to avoid referring to a component or project because of the case where headers are included from separate components along the relative path: mylib/foo.h includes mylib/deps/otherlib/bar.h. In that case, one might conclude the guidance doesn't apply because "otherlib" is a different component. but I suppose if entire dependent projects or headers are placed within the project (as opposed to consuming them from an external location) then, like it or not, those separate projects have now become an inseparable part of the project.

clarified this as:
"... files that exist at a relative path to the file containing the #include statement (from within the same component or project) "

... files that exist at a relative path to the file containing the `#include` statement (from within the same component or project)
@gdr-at-ms gdr-at-ms merged commit 6f50150 into isocpp:master Jul 25, 2020
@johnmcfarlane
Copy link
Contributor

Thanks @gdr-at-ms. This section is improved now. Unfortunately, @apenn-msft , the merged wording suffers from exactly the ambiguity I mentioned here.

These lines reintroduce use of problematic term, 'project'.

Whether or not an included file is from the same project is not sufficient to determine whether "" or <> is appropriate, but I suspect that readers will now be encouraged always to use "" -- even when a header search path was used to locate the header.

@apenn-msft
Copy link
Contributor Author

@johnmcfarlane I also wanted to avoid the use of terms like 'project' or 'component', but the statement is qualified as "locally relative":
"A file locally relative to foo.cpp in the same project"

If a header search path (<>) was used to locate a file at the same path where it exists relatively then the guidance applies, and "" should be used instead.
Can you think of a counter-example?

Example:
myproject/componentA/foo.h
myproject/componentA/bar.h

In this case, the guidance is recommending foo.h include bar.h using "", this seems correct to me and in the spirit of the guidance. While myproject could be on the header search path, the guidance makes it more portable: using "" to access bar.h relatively will work in either case (whether it's on the search path or not) and more concisely names bar.h as "the bar.h that is co-existent with foo.h" increasing likelihood that the correct bar.h is selected.

Note that in a case like:
myproject/componentA/foo.h
myproject/componentB/bar.h

Whether "" or <> is used (in context of this guideline) does depend on whether the reader considers foo.h to be relatively reachable from bar.h and that depends on their policy of using ".." in include paths. That decision is intentionally left up to the reader.

@Quuxplusone
Copy link
Contributor

I also wanted to avoid the use of terms like 'project' or 'component'

Couldn't you remove the words "project" and "component", then?

Nevertheless, the guidance is to use the quoted form for including files that exist at a relative path to the file containing the #include statement (from within the same component or project) and to use the angle bracket form everywhere else, where possible.

#include "foo.h" // A file locally relative to foo.cpp in the same project, use the "" form

@apenn-msft
Copy link
Contributor Author

apenn-msft commented Aug 13, 2020

@Quuxplusone, they were originally removed in response to @johnmcfarlane original comment; but see @gdr-at-ms comments:

"expressing clearly the desired structure."

where the worry was a possible interpretation that leaves readers coding against the header where it resides rather than the header from where it is authored (e.g. the difference between /usr/include/foo.h and myproj/foo.h).
We needed a name for this "authorship location" that satisfied gdr's desire to express "structure" and well... structure, component, library, project, these are all synonyms, so I just picked the most accessible/recognizable term.

I felt I was able to satisfy both gdr's comment by naming the structure ("project") and john's comment by stating that even within a "project" local relative-ness must apply if using "" (which I think addresses the issue with the original reason to remove it which is where someone might interpret needing to use "" anywhere within the project).

johnmcfarlane added a commit to johnmcfarlane/CppCoreGuidelines that referenced this pull request Aug 14, 2020
The examples in SF.12 are likely to encourage readers to always use the `""` form of `'#include` when including headers from the same project ([discussion](isocpp#1596 (comment))). However, in larger projects this may not always be appropriate; `<>` should be used for includes located via a header search path.

This proposed solution adds an example of the later, i.e. where `<>` is used to include a header from the same project.
@johnmcfarlane
Copy link
Contributor

@apenn-msft I fully understand. This isn't the first PR on this topic which was pulled in competing directions! I've opened another PR with an additional example:

#include <component_b/bar.h>     // A file in the same project located via a search path, use the <> form

I'd appreciate any feedback as this is perhaps the hardest example for many readers to comprehend.

@apenn-msft apenn-msft deleted the patch-1 branch August 15, 2020 02:51
@gdr-at-ms
Copy link
Contributor

I agree with @Quuxplusone and @johnmcfarlane about the use of the term "project". I continue to believe "component" is more appropriate than "project". Talk of "relative path" sounds too low level to provide an effective guidelines, at least what we want to convey -- which is why I asked the question earlier. Also, talk of "where the header is authored" fails to capture the common scenario that headers can be generated at build time.

@Quuxplusone - would you mind preparing a PR that reflects your suggested edit?

@apenn-msft
Copy link
Contributor Author

"component" is not a sufficient enough level to reason over because it is many-to-one to a "project". A project or "library" can consist of multiple "component"s; see @johnmcfarlane's example in #1664 which implies this interpretation as well.

Borrowing from the example in #1664, if you have structure:

some_library/componentA/componentB

The syntax for including componentB/bar.h from componentA/foo.h is impacted by this guidance; if reasoning at the "project" level (e.g. considering relativity from anywhere within some_library), you would apply the guidance as using the "" form. This is the better outcome since it increases the likelihood of locating the correct header (see my earlier example on when <> and "" can locate different headers even when used within the same project).

@Quuxplusone
Copy link
Contributor

@Quuxplusone - would you mind preparing a PR that reflects your suggested edit?

Opened #1666.

frozenca pushed a commit to frozenca/CppCoreGuidelines that referenced this pull request Nov 7, 2020
The examples in SF.12 are likely to encourage readers to always use the `""` form of `'#include` when including headers from the same project ([discussion](isocpp#1596 (comment))). However, in larger projects this may not always be appropriate; `<>` should be used for includes located via a header search path.

This proposed solution adds an example of the later, i.e. where `<>` is used to include a header from the same project.
@ecorm
Copy link

ecorm commented Apr 30, 2022

Sorry, but it's not clear which form of #include I should use as a library creator when including my own headers from within my own library.

Other practical matters aside, I find

#include <mylib/foo.hpp>

to be cleaner than

#include <../include/mylib/foo.hpp>

while considering the common case where *.cpp files live in a different directory than *.hpp files within the library project structure.

@ecorm
Copy link

ecorm commented Apr 30, 2022

I've grappled with this in my attempt of an answer at SO. Note that the question is specific to what library writers should do in their own library code. Comments are welcome.

@apenn-msft
Copy link
Contributor Author

apenn-msft commented Apr 30, 2022

@ecorm the relevant example is here:
#include "foo.h" // A file locally relative to foo.cpp in the same project, use the "" form #include "foo_utils/utils.h" // A file locally relative to foo.cpp in the same project, use the "" form

I was able to land on the word 'project', and some confusion is merited here, because we did have the majority of feedback center around what to do for "library writers", and understandably, it is challenging to define what is a "library" or "project" in C++. We went with "project" because it's a bit more general than "library", but there really is no such concept in C++ for a good reason: the overarching principle is to not segment the language into "library mode" and "user mode". it's all the same code, we want the rules to apply uniformly. (even though yes, we tend to loosely think of "project" as a collection of highly cohesive code that is typically provided as a portable unit)

In short that means the same rules for the library writer, as the user, as in the example above.
Your SO post does a good job of outlining the behavior differences we considered, but some of the advantages listed are what SF.12 seeks to avoid as ambiguity. For example, a user/compiler supplanting <foo/utils.h> with another one by changing the <>-form look-up order leads to more confusion and less portability (since as you mention, it relies on header search order). The aim of SF.12 is to get more portable code by recommending the ""-form consistently provide the same behavior across implementations, which is to provide "locally relative" look-up. That means when the library writer of libfoo provides foo.h and utils.h, they get a stronger/more portable way to say "include utils.h - the one I provided alongside foo.h", which less ambiguously locates a header than saying #include <foo/utils.h>.
That is one of the major ideas behind SF.12, is to provide a common interpretation for what the ""-form should mean so it's easy to interpret how the header is being located (although yep some compilers (Apple) can also tinker with the ""-form, but if we're to follow SF.12, compiler setups should and typically do by default interpret the ""-form as a local/relative header look-up)

@apenn-msft
Copy link
Contributor Author

apenn-msft commented Apr 30, 2022

that being said, it's also deliberately left up to the reader to interpret the degree to which "relative locality" applies, which speaks to your other point on SO re: at what point does one do "../../../unsightly/path/foo/foo.h" vs <foo/foo.h>. That also goes back to inability to say what constitutes a project (at which directory do you define the project split?) and intentionally leaves it up to the reader.
In practice, I think most folks commonly agree on same-level and downward relativity, e.g. #include "foo.h" and #include "utils/foo.h", and others will have varying opinions about how to interpret "../foo/utils.h". it depends on how much you're willing to trade off an unsightly include path vs using the include form that is less ambiguous. (for me personally, I tend to be ok with 1 or 2 levels up, but beyond that the unsightliness (i.e. ../../../) outweighs the disambiguation and I may no longer consider such a path feasibly "relative")

@ecorm
Copy link

ecorm commented Apr 30, 2022

@apenn-msft Thank for you the clarifications. Based on my current understanding, I'm now personally aiming to adopt the following convention:

  • When a library header file includes another header of the same library: Use #include "foo.h", #include "bar/foo.hpp", or #include "../bar/foo.hpp", depending on the relative location of the other header file.
  • When a library source (cpp) file includes one of the library's own headers: Use #include <somelib/foo.hpp> or #include <somelib/bar/foo.hpp>.

The former makes it clear that I want to include a header file that's bundled with the same library as the file doing the including.

The latter makes it possible for the library source (cpp) files to be compiled by being directly dropped into an application's build system, without having to maintain the same relative directory structure between the library's source and header files. It also avoids the #include "../include/somelib/foo.hpp" ugliness in favor of the cleaner #include <somelib/foo.hpp>.

When my library source files are being compiled using the library's CMake scripts (to generate a static/shared library), the CMake scripts are in control of the -I and -isystem flags and can thus ensure that the correct library headers path will be given top search priority.

@ecorm
Copy link

ecorm commented Apr 30, 2022

others will have varying opinions about how to interpret "../foo/utils.h"

This could lead to usage of the #include directive that appears puzzlingly inconsistent. Imagine a library with headers having the following structure:

somelib/
|
+--- core/
|      |
|      utilities.hpp
|
+--- widget/
       |
       + textbox.hpp
       |
       + combobox.hpp

In combobox.hpp, you could have:

#include "textbox.hpp"
#include <somelib/core/util.h> // as opposed to #include "../core/util.h"

Both of those headers logically belong to the same "project" (i.e the "somelib" library), yet they are included via different forms.

@apenn-msft
Copy link
Contributor Author

yep, consistency of intra-project include form is nice, but the implication of SF.12 is that it prioritizes portability and consistency of behavior across platforms where there is a trade-off. Where you can, use the more portable relative-local header look-up. If you can't (perhaps at the limit of unsightliness for ".."'s) then it's fine to fallback to using the <>-form. As you rightly point out, this may result in different include forms locating what should be the same header within the same project. SF.12 is saying it is more important to use the form that most uniquely locates a header, rather than the same form everywhere.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.