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