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