On Sun, Feb 2, 2014 at 8:00 AM, Ted Kremenek <[email protected]> wrote:
> On Jan 29, 2014, at 7:45 AM, Alexander Kornienko <[email protected]> > wrote: > > ... > > > I've renamed CheckerNameWrapper to CheckerName. As for the need in this > class in general, it serves as a guard against random StringRefs being > passed to BugType. This was suggested by Jordan (I'm not sure, that the > implementation is exactly what he imagined, though), and I think it's a > reasonable precaution. > > > Yes, it sounds good to me. ‘CheckerName’ is a big improvement over > ‘CheckerNameWrapper’, and we can always refine the terminology later. > > > >> ... >> There’s a single loop that goes and creates all the checker objects, >> which (I believe) includes both builtin checkers and ones loaded from >> plugins. The CheckerInfo object contains the full string of the checker. >> What if we changed the loop body to something like this: >> >> { >> checkerManager.setCheckerName((*I)->FullName; >> (*i)->Initialize(checkerMgr); >> } >> >> Then in CheckerManager, we can modify >> CheckerManager::registerChecker<T>() to consult that StringRef, and if it >> is there, use it to splat the name into the CheckerBase object. You then >> don’t have to touch the individual registerXXXChecker() functions at all. >> It’s less boilerplate for checker authors, and also less error prone. It >> is also extensible if we ever want to plumb in more data. I know it’s a >> bit gross since it’s meddling with some shared state in CheckerManager, but >> this initialization really only happens up front during startup. >> > > I was thinking about this alternative before, but chose the other one to > avoid dealing with a mutable shared state. If you prefer this option, I'm > happy to change it. > > > It’s tradeoffs, but I don’t see it as a big deal. CheckerManager isn’t > thread-safe when it is being mutated in any way, and all this work happens > during initialization. I’d rather do this then punish checker writers with > a bunch of redundant boilerplate. > > Jordan: what are your opinions on this approach? > > > >> Aside from that, is there a reason we want to traffic at all with >> CheckerNameWrapper (or rather, CheckerName) for calls to things like >> EmitBasicReport? > > > I've added an overload of the EmitBasicReport, which receives a pointer to > CheckerBase instead of the checker name. There's one corner-case though: > checkers, that implement several sets of checks (MallocChecker, > CStringChecker and a few others). These checkers need to pass the exact > checker name, which can't be replaced with a reference to the checker. > > > Ah, yes. This concept of “checks” and “checkers” aren’t totally > formalized (with the latter implementing the former). What “CheckerName” > corresponds to is the specific “Check” or “Checker Diagnostic”. I think > this is fine for now, but I think this concept will need to be refined over > time. > I can rename it to CheckName (the CheckerName class and all the related variables/methods as well) to avoid confusion. What do you think? > > > >> ... > > > It would be interesting , but, as I've said, we can't just replace the > checker name with a reference to the checker. > > > Yes, I forgot about this obviously important point. > So, is the patch ready to commit (with the optional CheckerName->CheckName rename)? ;)
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
