[cfe-commits] [Patch][Review Request]Fix for PR7287

[Ted Kremenek]

On Nov 2, 2010, at 2:25 PM, Jim Goodnow II wrote:

>> I see two alternative options:
>> 
>> (1) Have a restrictive interface in Checker, as I originally proposed, to clearly limit the scope of what Checker writers must think about.
>> 
>> (2) Remove NamedDecl::getIdentifier() and NamedDecl::getNameAsString().  This forces all clients of NamedDecl (both the analyzer and others) to explicitly reason about all cases. While this may "add" code in some places, at least the compiler will enforce that the client has explicitly reason about the the various cases.
> 
> Is the overlap between C, ObjectiveC and C++ a contributing factor? All of the experimental checks are grouped together at the moment having evolved from the original ObjectiveC memory checker. PR7287 arises because of checkers written for C running on C++ code. Is it time to split these and register only those checkers that are applicable to the specific code being analyzed and/or a specific option indicating which set of checks to use? Under that scenario, a pure C checker wouldn't need to worry about CXX expressions. If you want a checker to handle C++ as well as C, you need to aware of these issues. Checkers that start as pure C can be extended to work with C++ in the future.


Jim,

I'm not a real fan of that approach as a general strategy (although it certainly is applicable in a few cases).  All of the C checkers seems equally applicable in C++, and even the Objective-C checkers are applicable in the context of Objective-C++.  In other words, if the checker is Objective-C specific, it should only be enabled if the code is Objective-C or Objective-C++, but that doesn't solve the problems we're encountering with migrating the checkers and the rest of the analysis engine to work with C++.

I'd rather that checkers were engineered from the onset to work in all languages where they are applicable.  In the context of static analysis, that mostly means they just need be explicit about what they care about, and not fall into the trap of assuming something is true in all cases when it isn't.  The trick is not having booby traps in the APIs where checker writers can make mistakes.  With NamedDecl::getIdentifier(), this seems like a booby trap since checker writers aren't forced to consider the case where this is null unless it comes up in a segfault.  At least with NamedDecl::getName() the checker writer is forced to reason about what the care about.

The other approach I proposed was to essentially have the checker declaratively state what parts of the language they cared about (via invariants in the Visitor interface).  That's similar to your suggestion, but it's different than not having a checker work at all when the code is (say) C++.