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

Add SF.12, 3rd try #1570

Merged
merged 12 commits into from Feb 27, 2020
Merged

Add SF.12, 3rd try #1570

merged 12 commits into from Feb 27, 2020

Conversation

ChrisGuzak
Copy link
Contributor

No description provided.

@ChrisGuzak ChrisGuzak changed the title Add sf 12 2 Add SF.12, 3rd try Feb 22, 2020
CppCoreGuidelines.md Outdated Show resolved Hide resolved
CppCoreGuidelines.md Outdated Show resolved Hide resolved
CppCoreGuidelines.md Outdated Show resolved Hide resolved
CppCoreGuidelines.md Outdated Show resolved Hide resolved
@shaneasd
Copy link
Contributor

I agree with what I believe to be the intended reading of this rule. Is it sufficiently clear, though, that if angle brackets don't work the solution is to use quotes rather than change search paths? i.e. in the scenario where <helpers.h> doesn't work I could fix this by either adding the directory in question to the includes search path or by using quotes.

Perhaps clarifying this would be too much in the direction of hand-holding but I can imagine this cropping up in a code review where a less experienced developer has interpreted literally as "where you can" rather than "where you can without breaking other implied rules"

CppCoreGuidelines.md Outdated Show resolved Hide resolved
CppCoreGuidelines.md Show resolved Hide resolved
scripts/hunspell/isocpp.dic Outdated Show resolved Hide resolved
@ChrisGuzak
Copy link
Contributor Author

ChrisGuzak commented Feb 24, 2020

@shaneasd

... in the scenario where <helpers.h> doesn't work I could fix this by either adding the directory in question to the includes search path or by using quotes.

good point, I can see how some will be tempted to do this. in fact in the windows OS build "." is added to the include path making <> work in cases where normally it does not.

I'm open to edits that address this but maybe it does need to be.

@ChrisGuzak ChrisGuzak marked this pull request as ready for review February 24, 2020 16:39
@shaneasd
Copy link
Contributor

I'm open to edits that address this but maybe it does need to be.

If I think of a way to express this concisely I'll suggest it but at the moment I'm struggling.

@hsutter hsutter self-assigned this Feb 27, 2020
@hsutter hsutter merged commit 63ceef6 into isocpp:master Feb 27, 2020
@hsutter
Copy link
Contributor

hsutter commented Feb 27, 2020

Editors call: Great, thanks again for iterating on this and contributing it!

hsutter added a commit that referenced this pull request Feb 27, 2020
@johnmcfarlane
Copy link
Contributor

Sorry for the infrequent feedback and thanks for putting the time into adding this rule.

The title could be read the right way if only the details were correct. However, the advise is the same as in #1456 and I still suspect it's not quite right. I know a lot of people follow this practice so that's understandable. However, it isn't aligned with what our common toolchains do so I'm still concerned that it will cause confusion and perpetuate bad habits.

#include "helpers.h" // A project specific file, use "" form

Project-specific files may need to use either <> or "" depending on whether the user intends to locate helpers.h via a header search within the project scope or relative to the including file (respectively). The fact that the two files are within the confines of a single project is of no consequence.

Failing to follow this results in difficult to diagnose errors due to picking up the wrong file by incorrectly specifying the scope when it is included.

This would be true if the advise were correct. But again, using "" in an intra-project inclusion where <> is the correct thing to do is still going result in occasional errors. That's now worse if the user was following advise from the CCG.

Apart from anything else, how does this apply to libraries outside the project? They are projects too. Should they use <> or "" when including their own headers? From their point of view, they're including project-specific headers, so we're saying they should use "". But from the point of view of a depenent project, they should be using <>. How does one resolve this contradiction if "being in a project" is the way we're distinguishing files?

Library creators should put their headers in a folder and have clients include those files using the relative path #include <some_library/common.h>

The use of "relative" here is confusing because ""-style inclusion is relative to the including file, whereas the <> style is absolute.

Please read the tool-chain documentation (my emphasis):

  • GCC: "By default, the preprocessor looks for header files included by the quote form of the directive #include "file" first relative to the directory of the current file"
  • MSVC: "The quotation marks mean that the preprocessor first searches the directory that contains the parent source file."

Would anybody mind if I fix this or is everybody in disagreement with me? I ask because my posts on the subject in the previous PR didn't receive much feedback so I still don't know if I'm onto a losing battle here.

@shaneasd
Copy link
Contributor

shaneasd commented Mar 9, 2020

It seems to me like your objection is purely to // A project specific file, use "" form? If so, I agree. It adds a new concept (project ownership) in the example where it should really be illustrating the established concepts and I agree with you that the new concept it introduces is not strictly appropriate.

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.

None yet

6 participants