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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
