[PATCH] [analyzer][Review request] Improved checker naming in CFG dump.

Ted Kremenek kremenek at apple.com
Fri Feb 14 19:59:38 PST 2014



================
Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1741-1742
@@ -1740,3 +1740,4 @@
   if (!Errors.empty()) {
-    static SimpleProgramPointTag Tag("MallocChecker : DeadSymbolsLeak");
+    static SimpleProgramPointTag 
+           Tag(getCheckName().getName() + " : DeadSymbolsLeak");
     N = C.addTransition(C.getState(), C.getPredecessor(), &Tag);
----------------
Jordan Rose wrote:
> Jordan Rose wrote:
> > Ted Kremenek wrote:
> > > This is a small improvement, but it is still boilerplate.  It also doesn't enforce regularity.  It would be great if I could write:
> > > 
> > >   SimpleProgramPointTag Tag(getCheckName(), "DeadSymbolsLeak")
> > > 
> > > and then just have the constructor do the string formation.  Going one step further, it would be great if I could just write:
> > > 
> > >   SimpleProgramPointTag Tag(this, "DeadSymbolsLeak")
> > > 
> > > since "getCheckName()" can easily be called.  That's much simpler and regular. You then don't have to expose string concatenation to the user, and it allows us to change the internal representation of SimpleProgramPointTag later if we should so choose.
> > I agree, with the note that it should be a separate CheckerProgramPointTag class. ProgramPointTag is already virtual.
> Actually, well...these are statically constructed, which means it's a bit weird that they will use the package name of a particular Checker instance. I guess that's okay in general, but for checkers like MallocChecker that implement more than one check, we //shouldn't// do this, because the check whose name shows up here the first time may not even be enabled on the next run.
For me that's an argument about whether or not this particular instance of SimpleProgramPointTag should be a 'static' variable, not how they are constructed.  We do, however, rely on the pointer identity of the SimpleProgramPointTag object, so this argues for a collection of these tags, but that seems orthogonal to whether or not they use the package name or the Checker instance.  What do you think?


http://llvm-reviews.chandlerc.com/D2793



More information about the cfe-commits mailing list