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

[Ted Kremenek]

================
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);
----------------
Ted Kremenek wrote:
> 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?
Actually, I don't understand this comment.  These are just debugging aids, with the check producer (aka the checker) and some other "name" that's relevant for node tracking purposes.  The concept of a "check" doesn't even come in here.  Your point confused me a bit, which is why I made that comment before about static variables.  These are just tags in the ExplodedGraph, nothing more.  As long as we come up with consistent names for these tags for debugging purposes that's all that really matters.  Putting in the checker name makes perfect sense here, since it is the actual entity that is constructing the nodes.

================
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);
----------------
Ted Kremenek wrote:
> Ted Kremenek wrote:
> > 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?
> Actually, I don't understand this comment.  These are just debugging aids, with the check producer (aka the checker) and some other "name" that's relevant for node tracking purposes.  The concept of a "check" doesn't even come in here.  Your point confused me a bit, which is why I made that comment before about static variables.  These are just tags in the ExplodedGraph, nothing more.  As long as we come up with consistent names for these tags for debugging purposes that's all that really matters.  Putting in the checker name makes perfect sense here, since it is the actual entity that is constructing the nodes.
Ignore this comment.  I was confused.


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