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

[Jim Goodnow II]

At 06:53 PM 11/4/2010, Ted Kremenek wrote:

>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++.

True, the two checkers that had the problem were CStringChecker and 
PthreadChecker which certainly can apply across the board.

>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++.

So, there is a place for some discrimination between checkers based 
on language variant. I was also thinking in terms of efficiency. As 
more checkers are written for C++ specific analysis, having them run 
on C or ObjectiveC is just burning cycles needlessly. A checker that 
is more universal can be registered with multiple language variants.

>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++.

Yes, I can certainly see the value in that.