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

[Kim Viggedal via Phabricator via cfe-commits]

vingeldal added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.cpp:20
+namespace {
+AST_MATCHER(VarDecl, isLocalVarDecl) { return Node.isLocalVarDecl(); }
+} // namespace
----------------
JonasToth wrote:
> This matcher already exists either as global matcher or in some other check. please refactor it out.
I reworked the implementation significantly and just decided I didn't need this matcher at all.


================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.cpp:39
+      << MatchedDecl;
+  diag(MatchedDecl->getLocation(), "insert 'const '", DiagnosticIDs::Note)
+      << FixItHint::CreateInsertion(MatchedDecl->getLocation(), "const ");
----------------
JonasToth wrote:
> `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.
My priority is getting a check merged, fix-it can be dropped from this patch.


================
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]
----------------
JonasToth wrote:
> 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.
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70265