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