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
