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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman 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;
----------------
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;
  }
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70265