Page MenuHomePhabricator

[clang-tidy] Add CppCoreGuidelines I.2 "Avoid non-const global variables" check
Needs ReviewPublic

Authored by vingeldal on Nov 14 2019, 11:32 AM.

Details

Summary

Cpp Core Guideline I.2, a.k.a "Avoid non-const global variables"
For detailed documentation, see:
https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#i2-avoid-non-const-global-variables

Event Timeline

vingeldal created this revision.Nov 14 2019, 11:32 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 14 2019, 11:32 AM
JonasToth added inline comments.Nov 14 2019, 5:11 PM
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.cpp
21

This matcher already exists either as global matcher or in some other check. please refactor it out.

40

const fixing is a complicated issue. I would rather not do this now, as there is currently a patch in flight that does the general case.
If you really want it, there is a util for that in clang-tidy/util/ that is able to do some const-fixing.

clang-tools-extra/docs/ReleaseNotes.rst
97

Please describe the check with one sentence and sync that with the check documentation.

clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-avoid-non-const-global-variables.rst
7

Please adjust the documentation here as well :)

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-avoid-non-const-global-variables.cpp
4

Please add test-cases for pointers and references of builtins (int and the like) and classes/structs/enums/unions, function pointers and member function/data pointers.
We need to consider template variables as well (i think a c++14 feature).

I am not sure about the consteval support in clang for c++20, but for each feature from a newer standard its better to have a additional test-file that will use this standard.

Hi vingeldal,

thanks for you contribution and welcome to LLVM :)
If you have any questions or the like, feel free to ask!

Best Regards, Jonas

vingeldal updated this revision to Diff 234863.Dec 20 2019, 5:05 AM
vingeldal marked 5 inline comments as done.

Updating D70265: [clang-tidy] Add clang tidy check I.2 to cppcoreguidelines

Adressed all comments:

  • Extended teseting
  • Updated documentation
  • Removed redundant matcher which was added in initial patch
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.cpp
21

I reworked the implementation significantly and just decided I didn't need this matcher at all.

40

My priority is getting a check merged, fix-it can be dropped from this patch.

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-avoid-non-const-global-variables.cpp
4

I made the test somewhat more exhaustive. Would you say consteval can be dealt with in a separate patch later, when C++20 is officially released, or would it be preferable to just hold this patch until then and incorporate that change in this patch?

JonasToth added inline comments.Dec 21 2019, 3:22 AM
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.cpp
27

Please write the comments as full sentences with punctuation. I think ... only the data they reference ....

33

The matchers here are not pointers/references. Therefore, the llvm guidelines say they should not be const.

65

each of those ifs should return early, or could multiple match?
If only one can match the structure could be changed a bit to

if (const auto* Variable = Result.Nodes.getNodeAs<VarDecl>("non-const_variable")) {
    diag(...);
    return;
}

If you change the order of the ifs in the order of matches you expect to happen most, this should be a bit faster as well, as retrieving the matches introduces some overhead that is not relevant in the current situation.

If you keep only one statement within the if you should ellide the braces, per coding convention.

clang-tools-extra/docs/ReleaseNotes.rst
107

The official name is "C++ Core Guidelines", i think we should stick to that.
And please ellide the new empty line above.

clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-avoid-non-const-global-variables.rst
7

Please add a link to the affected section and provide a small example (can be the example from the guidelines) what is diagnosed and why.

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-avoid-non-const-global-variables.cpp
47

diagnosing those might be undesired. maybe having an option to enable/disable this would be nice?
We should try to allow reducing the noise level of clang-tidy.

vingeldal marked 7 inline comments as done.

Updating D70265: [clang-tidy] Add clang tidy check I.2 to cppcoreguidelines

  • Added an option to not check member variables.
  • Add checking for member variables referencing or pointing to non-const data.
  • Added some early returns in check implementation.
  • Expanded documentation, with an example and link to C++ Core Guidelines
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.cpp
65

There could be multiple matches but there could still be some early returns.
An example of a multi match would be:

namespace {
int i = 0;
}
int * i_ptr = &i;

There would be two warnings for i_ptr, since it is:

  1. A global non-const variable it self and...
  2. because it globally exposes the non-const variable i.

I'll add early returns where possible.

...Now that I think about it I realize I'v missed checking for member variables referencing or pointing to non-const data,
I'll add that tigether with some more testing.

clang-tools-extra/docs/ReleaseNotes.rst
107

I'll remove the empty line, I must ask about this formatting though.
I see multiple examples both with and without the empty line, so I just tried to be consistent with what most people seem to do.
What's the rule for knowing when to use one and when not to?

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-avoid-non-const-global-variables.cpp
47

I can see your point and I agree that an option would be a good idea.
It isn't obvious that members variable could be considered global variables.
I'll make the current behavior the default though, since I got the impression
that in most aligned with the intent of the guideline.

aaron.ballman added inline comments.Dec 26 2019, 9:09 AM
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.cpp
30

Why are you filtering out anonymous namespaces?

65

Based on my reading of the C++ core guideline, I think there should be a different two diagnostics. One for the declaration of i and one for the declaration of i_ptr, because of this from the rule:

The rule against global variables applies to namespace scope variables as well.

90

I'd sink the local variables into the if statement:

if (const auto *Var = Result.Nodes.getNodeAs<VarDecl>("non-const_variable")) {
  ...
}

(If you don't want to wrap lines, you can make the string literals a bit shorter.)

107

pointed to -> pointed-to

121

Spurious semicolon

124–125

Can re-flow this string literal.

131

Spurious semicolon.

134

Can re-flow this string literal.

135

pointed to -> pointed-to

clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-avoid-non-const-global-variables.rst
10

Add backticks around things that should be in code font like NoMembers.

45

Backticks here as well.

53

Rather than NoMembers, how about going with IgnoreDataMembers?

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-avoid-non-const-global-variables-NoMembers.cpp
25 ↗(On Diff #235176)

Please add a newline to the end of the file.

sylvestre.ledru added inline comments.
clang-tools-extra/docs/clang-tidy/checks/list.rst
407

list.rst changed, you should update this!
Thanks

vingeldal updated this revision to Diff 235474.Dec 28 2019, 5:31 AM
vingeldal marked 12 inline comments as done.

Updating D70265: [clang-tidy] Add clang tidy check I.2 to cppcoreguidelines

  • Updated list.rst
  • changed: "pointed to" -> "pointed-to"
  • CHanged option name: NoMembers -> IgnoreDataMembers
  • Moved matcher declaration into if-statement
  • Fixed some formatting issues
  • Removed forgotten FIXME-comments that were no longer relevant
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.cpp
30

If it's in an anonymous namespace it's no longer globally accessible, it's only accessible within the file it is declared.

65

If the variable i was in a named namespace it would be matched by a diagnostic, i_ptr is matched by another diagnostic for pointer providing global access to non-const data (as well as the matcher for global non-const variables)

124–125

Sorry, I don't understand what you mean by re-flow.

clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-avoid-non-const-global-variables.rst
53

Yes, that's much better, thanks.

clang-tools-extra/docs/clang-tidy/checks/list.rst
407

I'm a bit concerned with this previous change of of list.rst.

Now that I need to add a check to this file, I don't know what severity to give it. I can't find any definition of severity levels and I really want to avoid getting into a long discussion, or having different reviewers not agreeing on my patch, concerning what severity we should give this check.
Is there any way I can find some kind of guideline on how to set the severity to avoid an opinionated discussion about severity level?

aaron.ballman added inline comments.Dec 28 2019, 6:09 AM
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.cpp
30

It is, however, at namespace scope which is what the C++ Core Guideline suggests to diagnose. Do you have evidence that this is the interpretation the guideline authors were looking for (perhaps they would like to update their suggested guidance for diagnosing)?

There are two dependency issues to global variables -- one is surprising linkage interactions (where the extern nature of the symbol is an issue across module boundaries) and the other is surprising name lookup behavior within the TU (where the global nature of the symbol is an issue). e.g.,

namespace {
int ik;
}

void f() {
  for (int ij = 0; ik < 10; ij++) { // Oops, typo, but still compiles
  }
}

That's why I think the guideline purposefully does not exclude things like anonymous namespaces.

124–125

Sorry for not being clear -- the string literal spans three lines when it only needs to span two by re-writing the literal. e.g.,

"member variable %0 provides global access to non-const type, "
"consider making the pointed-to data const"
clang-tools-extra/docs/clang-tidy/checks/list.rst
407

I'd like to follow that discussion so that I can be consistent with my review advice, too.

vingeldal marked an inline comment as done.Dec 28 2019, 7:37 AM
vingeldal added inline comments.
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.cpp
124–125

Ah, yes that's looks better of course. Will fix that in coming update.

vingeldal marked an inline comment as done.Dec 28 2019, 8:04 AM
vingeldal added inline comments.
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.cpp
30

I don't have any evidence. To me the guideline still looks a bit draft-like, so I just tried my best guess as to the intent of the guideline.
In reading the guideline I get the impression that the intent is to avoid globally accessible data which is mutable,
mainly for two reason:

  • It's hard to reason about code where you are dependent upon state which can be changed from anywhere in the code.
  • There is a risk of race conditions with this kind of data.

Keeping the variable in an anonymous namespace seems to deals with the issues which I interpret to be the focus of this guideline.
Consider that the guideline is part of the interface section. If the variable is declared in an anonymous namespace there is no risk that a user of some service interface adds a dependency to that variable, since the variable will be a hidden part of the provider implementation.

Admittedly the wording doesn't mention an exception for anonymous namespaces, and the sentence you have referenced seems to suggest that any non-const variable in namespace scope should be matched.
I'm guessing though, that was intended to clarify that a variable is still global even if in a (named) namespace, because that would not have been an obvious interpretation otherwise.
Without the referenced sentence one might interpret only variables outside of namespace scope as global.
Arguably a variable in namespace scope isn't globally declared, though it is globally accessible, via the namespace name. I think the global accessibility is the reason for dragging in variables from namespace scope and if that is the case then we shouldn't also need to worry about anonymous namespaces.
This should probably be clarified in the actual guideline.

clang-tools-extra/docs/clang-tidy/checks/list.rst
407

Good point!

I propose this patch to explain
https://reviews.llvm.org/D71963

this is a lazy copy and paste of
https://github.com/Ericsson/codechecker/blob/master/config/config.md

clang-tools-extra/docs/clang-tidy/checks/list.rst
411

It should not be there. It won't be referenced on the list.

If you are uncomfortable setting a severity, leave it empty but it should clearly move above, not on the toctree

vingeldal updated this revision to Diff 235483.Dec 28 2019, 9:50 AM

Updating D70265: [clang-tidy] Add clang tidy check I.2 to cppcoreguidelines

  • Adjusted the line breaks of some string literals.
  • Modified list.rst
vingeldal marked 2 inline comments as done.Dec 28 2019, 9:54 AM
vingeldal added inline comments.
clang-tools-extra/docs/clang-tidy/checks/list.rst
407

Great, will make sure to contribute some feedback to that.

aaron.ballman added inline comments.Dec 28 2019, 12:45 PM
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.cpp
30

This should probably be clarified in the actual guideline.

This sort of thing comes up with coding guidelines from time to time and the way we usually handle it is to ask the guideline authors for guidance. If they come back with an answer, then we go that route (while the authors fix the text) and if they don't come back with an answer, we muddle our way through. Would you mind filing an issue with the C++ Core Guideline folks to see if they can weigh in on the topic? If they don't respond in a timely fashion, I think it would make more sense to err on the side of caution and diagnose declarations within anonymous namespaces. This matches the current text from the core guideline, and we can always relax the rule if we find it causes a lot of heartache in the wild. (If you have data about how often this particular aspect of the check triggers on large real-world code bases, that could help us make a decision too.)

vingeldal marked an inline comment as done.Dec 29 2019, 2:59 AM
vingeldal added inline comments.
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.cpp
30

I sent a pull request to the guide lines, suggesting a clarification. If that is addressed in the near future I guess we can go on what they say about the pull request. If it takes to long I'll just modify the code to warn in anonymous namespace as well.
https://github.com/isocpp/CppCoreGuidelines/pull/1553

aaron.ballman added inline comments.Dec 29 2019, 11:51 AM
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.cpp
30

Thank you, I think that approach makes sense.

JonasToth added inline comments.Dec 30 2019, 6:04 AM
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.cpp
111

missing return?

clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-avoid-non-const-global-variables.rst
7

I would prefer if the I.2 of C++ Core Guidelines is a link and to remove the link-string below this paragraph.

30

This is technically not valid code, as the pointer needs to be initialized and a ; is missing. I think that should be fixed to not mislead inexperienced programmers.

vingeldal updated this revision to Diff 235600.Dec 30 2019, 6:59 AM
vingeldal marked 2 inline comments as done.

Updating D70265: [clang-tidy] Add clang tidy check I.2 to cppcoreguidelines

  • Fixed code example in documentation to make it valid code.
  • Fixed the documentation link to I.2 C++ Core Guidelines.
vingeldal marked an inline comment as done.Dec 30 2019, 7:03 AM
vingeldal added inline comments.
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.cpp
111

No, one of the below matchers may also be valid, in case we have a non-const member variable which is also a pointer or reference to non-const data.

There are many comments not marked as done. could you please do that with addressed issues? It is hard to otherwise see whats still left.

clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.cpp
111

I see. Please add a FALLTHROUGH comment or the like, as it breaks the pattern a lot and is not obvious from local code.

vingeldal marked 10 inline comments as done.Dec 30 2019, 8:08 AM
vingeldal marked 2 inline comments as done.
vingeldal marked an inline comment as done.Jan 6 2020, 1:53 AM
vingeldal added inline comments.
clang-tools-extra/docs/clang-tidy/checks/list.rst
407

It seems to me like this list is still generated the old way if one uses add_new_check.py to add a new check.
@sylvestre.ledru could you please also adapt that script to generate the table instead of the list?

lebedev.ri retitled this revision from [clang-tidy] Add clang tidy check I.2 to cppcoreguidelines to [clang-tidy] Add CppCoreGuidelines I.2 "Avoid non-const global variables" check.Jan 6 2020, 2:03 AM
vingeldal updated this revision to Diff 243757.Tue, Feb 11, 1:07 AM

Updating D70265: [clang-tidy] Add CppCoreGuidelines I.2 "Avoid non-const global variables" check

Created the requested pull request to C++ Core guidelines and since it is now merged this patch is updated accordingly.

  • Anonymous namespaces are no longer ignored by the check as guideline editors explained that was not wanted.
  • The option to ignore data members was reomved, they are now always ignored.
  • Documentation was updated to mention that this check also covers C++ COre Guideline R.6, since R.6 is simply a duplication of I.2.

The pull request to C++ Core guidelines have been merged: https://github.com/isocpp/CppCoreGuidelines/pull/1553

Herb set me straight regarding the anonymous namespaces so I had to change that.
While waiting for the pull request to be reviewed I looked at more guidelines and also changed my mind about optionally ignoring data members, so I removed that option and made this check always ignore data members.
Data members are probably better dealt with in separate rules after all.

Sorry for the rather significant changes coming in this late when you all have already put so much time into reviewing, at least the check changed to be smaller in scope -which felt like the right way to go.