[PATCH] Expose the name of the checker producing each diagnostic message.

[Alexander Kornienko]

On Sun, Feb 2, 2014 at 8:00 AM, Ted Kremenek <kremenek at apple.com> wrote:

> On Jan 29, 2014, at 7:45 AM, Alexander Kornienko <alexfh at google.com>
> 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)? ;)
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140202/381ff40a/attachment.html>