[PATCH] D70265: [clang-tidy] Add CppCoreGuidelines I.2 "Avoid non-const global variables" check

[Kim Viggedal via Phabricator via cfe-commits]

vingeldal added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.cpp:55-71
+  if (const auto *Reference =
+          Result.Nodes.getNodeAs<VarDecl>("reference_to_non-const")) {
+    diag(Reference->getLocation(),
+         "variable %0 provides global access to non-const type, consider "
+         "making the referenced data const")
+        << Reference; // FIXME: Add fix-it hint to Reference
+    return;
----------------
aaron.ballman wrote:
> vingeldal wrote:
> > aaron.ballman wrote:
> > > I think these cases should be combined to avoid duplication.
> > > ```
> > > if (const auto *VD = Result.Nodes.getNodeAs<VarDecl>("whatever")) {
> > >   diag(VD->getLocation(), "variable %0 provides global access to a non-const object; considering making the %select{referenced|pointed-to}1 data 'const'") << VD << VD->getType()->isReferenceType();
> > >   return;
> > > }
> > > ```
> > > the matcher needs to be changed to bind to the same id for both cases.
> > > 
> > > Note, this rewords the diagnostic slightly to avoid type/object confusion (the variable provides access to an object of a non-const type, not to the type itself).
> > You mean just the pointer and reference cases right? That matcher seems to get much more complicated, I'm having some trouble accomplishing that. Are you sure that's necessary? What would the cases of duplication be?
> > You mean just the pointer and reference cases right? 
> 
> Yup!
> 
> > Are you sure that's necessary? What would the cases of duplication be?
> 
> Not strictly necessary, so if this turns out to be hard, I'm fine skipping it. However, I was thinking it would be something like:
> ```
> // Registering checks
> Finder->addMatcher(GlobalReferenceToNonConst.bind("qual-non-const"), this);
> Finder->addMatcher(GlobalPointerToNonConst.bind("qual-non-const"), this);
> 
> // In check
> if (const auto *VD = Result.Nodes.getNodeAs<VarDecl>("qual-non-const")) {
>   diag(VD->getLocation(),
>           "variable %0 provides global access to a non-const object; considering making the %select{pointed-to|referenced}1 data 'const'") << VD << VD->getType()->isReferenceType();
>   return;
>   }
> ```
Oh, I didn't understand I could just keep the two matchers and use the same binding string. I should have read more carefully.
Sure I'll do that, its no effort and it looks nicer.


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-avoid-non-const-global-variables.rst:33
+    char h = 0;
+  }
+
----------------
aaron.ballman wrote:
> This isn't legal code either.
> 
> You may want to run the example through the compiler to catch these sort of things.
Oops, will make sure and do that next time to save us all some time and me some embarrassment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70265