[cfe-commits] [Patch][Review Request]Fix for PR7287
Jim Goodnow II
jim at thegoodnows.net
Thu Nov 4 20:44:41 PDT 2010
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.
>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.
More information about the cfe-commits