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

[Jim Goodnow II]

At 11:57 AM 11/2/2010, Ted Kremenek wrote:

>On Nov 2, 2010, at 7:01 AM, Douglas Gregor wrote:
>
> > Sure, but why special-case simple function names? I'd bet that 
> checkers looking at function names don't check for the context of 
> the function (translation unit vs. namespace vs. class) or, if it 
> was an overloaded function, that we have the correct signature. 
> Shall we limit those, too?
> >
> > Why is a checker that looks at, say, attributes on callee 
> functions more complicated than one that looks at names? The AST is 
> simple in both cases, but you are suggesting that the former 
> require the advanced interface while the latter uses the simpler 
> one... even though, based on the issues I mentioned above, it's 
> more likely that the former will actually work :)
> >
> >> For Checkers that care about monitoring all of these cases, we 
> do have a Checker::VisitGenericCallExpr() that they can implement
> >
> >>
> >
> > Sure, but the same question crops up again: what defines the 
> simple case, and why?
>
>What defines the simple case is what I have seen from 
>experience.  Most checkers just care about matching to simple names.
>
>My concern is about the ease of writing *correct* checkers with 
>minimal code.  I want checker writers to always know what they 
>explicitly must be reasoning about, and no more.  Defining a 
>restrictive interface in Checker allows checker writers to exactly 
>know what invariants they are coding to.
>
>I can buy the argument that checkers should reason about the general 
>case for names, and then explicitly match on what they care 
>about.  From this design point, checkers should always use 
>NamedDecl::getDeclName().  Unfortunately, we vend 3 APIs for getting 
>the name of a NamedDecl: getIdentifier(), getNameAsString(), and 
>getDeclName().  The first seems very error prone (as we have seen), 
>the second is redundant, and the third really is the one API that 
>everyone should probably be using.
>
>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.