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