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

[Nithin VR via Phabricator via cfe-commits]

vrnithinkumar added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrChecker.cpp:110
+  SmartPtrChecker *Checker = Mgr.registerChecker<SmartPtrChecker>();
+  NullDereferenceBugTypePtr = &Checker->NullDereferenceBugType;
 }
----------------
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?


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:352
+// Gets the SVal to track for a smart pointer memory region
+SVal SmartPtrModeling::getSValForRegion(const CallEvent &Call,
+                                        CheckerContext &C) const {
----------------
xazax.hun wrote:
> I am a bit unhappy with this function.
> I feel like it does not really have a purpose. It returns the null constant for default constructor calls, for every other case it just returns the first argument. The branch we have in this function would completely go away if we would inline this to all the call sites and the result would be a single line each place. I'd rather just remove this function.
Also we have to assert that we are adding a pointer value to TrackedRegionMap.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84600