<div dir="ltr"><div class="gmail_extra"><br><div class="gmail_quote">On Sun, Feb 2, 2014 at 8:00 AM, Ted Kremenek <span dir="ltr"><<a href="mailto:kremenek@apple.com" target="_blank">kremenek@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div class="im">On Jan 29, 2014, at 7:45 AM, Alexander Kornienko <<a href="mailto:alexfh@google.com" target="_blank">alexfh@google.com</a>> wrote:<br>
</div><div><div class="im"><br><blockquote type="cite"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
...
</blockquote><div><br></div><div>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.</div>
</div></div></div></blockquote><div><br></div></div><div>Yes, it sounds good to me.  ‘CheckerName’ is a big improvement over ‘CheckerNameWrapper’, and we can always refine the terminology later.</div><div class="im"><br><blockquote type="cite">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
...<br>
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:<br>


<br>
{<br>
      checkerManager.setCheckerName((*I)->FullName;<br>
      (*i)->Initialize(checkerMgr);<br>
}<br>
<br>
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.<br>

</blockquote><div><br></div><div>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.</div></div></div>
</div></blockquote><div><br></div></div><div>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.</div>
<div><br></div><div>Jordan: what are your opinions on this approach?</div><div class="im"><br><blockquote type="cite"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div> </div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
Aside from that, is there a reason we want to traffic at all with CheckerNameWrapper (or rather, CheckerName) for calls to things like EmitBasicReport? </blockquote><div><br></div><div>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.</div>
</div></div></div></blockquote><div><br></div></div><div>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.</div>
</div></div></blockquote><div><br></div><div>I can rename it to CheckName (the CheckerName class and all the related variables/methods as well) to avoid confusion. What do you think?</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div style="word-wrap:break-word"><div><div class="im"><br><blockquote type="cite"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">...</blockquote><div><br></div><div>It would be interesting , but, as I've said, we can't just replace the checker name with a reference to the checker.</div>
</div></div></div></blockquote><div><br></div></div><div>Yes, I forgot about this obviously important point.</div></div></div></blockquote><div><br></div><div>So, is the patch ready to commit (with the optional CheckerName->CheckName rename)? ;)</div>
<div> </div></div>
</div></div>