[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:20
+namespace {
+AST_MATCHER(VarDecl, isLocalVarDecl) { return Node.isLocalVarDecl(); }
+} // namespace
----------------
This matcher already exists either as global matcher or in some other check. please refactor it out.


================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.cpp:39
+      << MatchedDecl;
+  diag(MatchedDecl->getLocation(), "insert 'const '", DiagnosticIDs::Note)
+      << FixItHint::CreateInsertion(MatchedDecl->getLocation(), "const ");
----------------
`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.


================
Comment at: clang-tools-extra/docs/ReleaseNotes.rst:97
 
+- New :doc:`cppcoreguidelines-avoid-non-const-global-variables
+  <clang-tidy/checks/cppcoreguidelines-avoid-non-const-global-variables>` check.
----------------
Please describe the check with one sentence and sync that with the check documentation.


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-avoid-non-const-global-variables.rst:6
+
+FIXME: Describe what patterns does the check detect and why. Give examples.
----------------
Please adjust the documentation here as well :)


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-avoid-non-const-global-variables.cpp:3
+
+int nonConstVariable = 0;
+// CHECK-MESSAGES:  warning: variable 'nonConstVariable' is non-const and global [cppcoreguidelines-avoid-non-const-global-variables]
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70265