Skip to content

Add clarifying example to SF.12 #1664

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Aug 16, 2020
Merged

Add clarifying example to SF.12 #1664

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CppCoreGuidelines.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19306,6 +19306,7 @@ Nevertheless, the guidance is to use the quoted form for including files that ex
#include <some_library/common.h> // A file that is not locally relative, included from another library; use the <> form
#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
#include <component_b/bar.h> // A file in the same project located via a search path, use the <> form
Copy link
Contributor

@apenn-msft apenn-msft Aug 14, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for following up on this.
the wording should be made mutually exclusive i.e.:
"A file that is not relative to foo.cpp, located using the header search path; use the <> form"

in the case where the file can be both located using "" and <>, the guidance should be unambiguous and the relative path should be preferred.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in the case where the file can be both located using "" and <>, [...] the relative path should be preferred.

I would caution against having any guideline of the form "prefer X over Y when physically possible." The goal of guidelines (and programming in general) is to pick the semantics you want and then massage the physical situation until it matches the semantics — not to observe the existing physical situation and then pick semantics to match the physics.

Example of picking the semantics first: "Including <mylib/all.h> should work the same as including both <mylib/alpha.h> and <mylib/beta.h>. So <mylib/all.h> should #include <mylib/alpha.h>, not #include "mylib/alpha.h" (*)." Then make the physics match: "When building mylib, or anything that depends on it, I need to make sure that target_include_directories contains the parent of mylib's include/mylib/ directory, so that my angle brackets will work."

Example of picking the physics first: "I'm writing two files, one named all.h and one named alpha.h, and the former needs to #include the latter." And making the semantics match the physics: "They live together in the same directory, so I can use a relative path to #include "alpha.h", so I'll just do that."

(* — Because #include "mylib/alpha.h" might not do the same thing as #include <mylib/alpha.h>.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Example of picking the semantics first: "Including <mylib/all.h> should work the same as including both <mylib/alpha.h> and <mylib/beta.h>. So <mylib/all.h> should #include <mylib/alpha.h>, not #include "mylib/alpha.h" (*)."

In this case you would include "alpha.h" (not "mylib/alpha.h") because alpha is relative to all.h. I would say that the physics are a part of the semantics; as in the last PR, gdr mentions that "structure" is important and this is why we added the term 'project' and why it is important to consider the physical layout and relation of the files being included.

If not, then consider the case in your footnote:
The user places "mylib" alongside their 'user.cpp' file:
usercode/user.cpp
usercode/mylib/all.h
usercode/mylib/alpha.h
usercode/mylib/beta.h

And there happens to also be a 'mylib' on the header search path (for example, the compiler installed mylib is 1.2 and the user wants to locally use 1.3; or for example it's a commonly named library that is likely to collide, like 'utils').

As you noted, in this case "mylib/all.h" is different from <mylib/all.h> and when the user goes to #include "mylib/all.h" if the mylib author did not follow guidance and did not prefer to relatively include alpha and beta from all.h, then the user is going to get the wrong headers.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that "alpha.h" is most robust here. But also I agree that there are two ways here to make the header available to the source file and only after determining which way is being used should this guidance kick in. We could provide orthogonal guidance to say that "" is preferable because it's less ambiguous, but that's different to the main aim of SF.12.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense, split it into separate guidance:
1: if you use #include "alpha.h", alpha.h must exist at a relative path
2: if you can include alpha.h from a relative path, prefer to; this will increase the likelihood the correct file is located

Copy link
Contributor

@apenn-msft apenn-msft Aug 15, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realize now the title is already making the recommendation:

If we split the rule, we'd also need to revise the current SF.12. I'm kind of partial to just leaving it as-is, but in either case, for this PR off the current SF.12, it'd be good to make the example clear that <component_b/bar.h> is not also available at a relative path.


##### Note

Expand Down