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

[Ted Kremenek]

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.