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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.cpp:26
+          hasType(isConstQualified()),
+          hasType(referenceType()), // References can't be changed, only data
+                                    // they reference can be changed
----------------
Please write the comments as full sentences with punctuation. I think `... only the data they reference ...`.


================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.cpp:32
+
+  const auto GlobalReferenceToNonConst =
+      varDecl(hasGlobalStorage(), hasType(referenceType()),
----------------
The matchers here are not pointers/references. Therefore, the llvm guidelines say they should not be `const`.


================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.cpp:64
+
+  if (Variable) {
+    diag(Variable->getLocation(), "variable %0 is non-const and globally "
----------------
each of those `if`s 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 `if`s 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.


================
Comment at: clang-tools-extra/docs/ReleaseNotes.rst:107
+
+  Finds non-const global variables as described in check I.2 of cpp core
+  guidelines.
----------------
The official name is "C++ Core Guidelines", i think we should stick to that.
And please ellide the new empty line above.


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-avoid-non-const-global-variables.rst:6
+
+Finds non-const global variables as described in check I.2 of cpp core
+guidelines.
----------------
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.


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-avoid-non-const-global-variables.cpp:46
+  int nonConstMemberVariable = 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: member variable 'nonConstMemberVariable' is globally accessible and non-const, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
+  const int constMemberVariable = 0;
----------------
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70265/new/

https://reviews.llvm.org/D70265