[PATCH] D84600: [Analyzer] Support note tags for smart ptr checker

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrChecker.cpp:110
+  SmartPtrChecker *Checker = Mgr.registerChecker<SmartPtrChecker>();
+  NullDereferenceBugTypePtr = &Checker->NullDereferenceBugType;
 }
----------------
vrnithinkumar wrote:
> xazax.hun wrote:
> > NoQ wrote:
> > > vrnithinkumar wrote:
> > > > NoQ wrote:
> > > > > Wait, i don't understand again. You're taking the bug type from that checker and using it in that checker. Why did you need to make it global? I thought your problem was about capturing the bug type from the //raw// null dereference checker?
> > > > > Wait, i don't understand again. You're taking the bug type from that checker and using it in that checker. Why did you need to make it global? I thought your problem was about capturing the bug type from the //raw// null dereference checker?
> > > > 
> > > > The check for bug type is in `SmartPtrModeling` but the bug type is defined in `SmartPtrChecker`
> > > Aha, ok, i misunderstood again. So i guess we didn't need a new header then. That said, it's an interesting layering violation that we encounter in this design: it used to be up to the checker to attach a visitor and so should be activating a note tag, but note tags are part of modeling, not checking.
> > > 
> > > I guess that's just how it is and it's the responsibility of the checkers to inform the modeling about bug types. I guess the ultimate API could look like `BugReport->activateNoteTag(NoteTagTag TT)` (where `TT` is a crying smiley that cries about "tag tags" T_T). But that's a story for another time.
> > I hope we will be able to figure out a nicer solution at some point. But I do not have a better alternative now, so I am ok with committing as is.
> > 
> > The API Artem is suggesting looks good to me (although it is out of scope for the GSoC). But I think that might not solve the problem of how multiple checkers should share the information about those tags. (Or at least I do not see how.)
> > 
> > Note that if we had both checks in the same file we might be able to work this problem around using `CheckerManager::getChecker`. I am not suggesting merging them, only wanted to point out.
> >  So i guess we didn't need a new header then.
> So we should remove the `NullDereference.h` and add the inter checker api to get the BugType to `SmartPtr.h`?
> 
> >  I guess the ultimate API could look like `BugReport->activateNoteTag(NoteTagTag TT)`
> Do we have to make these api changes on this review? Or some follow up changes?
> So we should remove the `NullDereference.h` and add the inter checker api to get the BugType to `SmartPtr.h`?
Yes.

> Do we have to make these api changes on this review? Or some follow up changes?
You don't have to do this at all; i think you have enough stuff on your plate. But if you like you can put up another patch later.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84600