[cfe-commits] [Patch][Review Request]Fix for PR7287
Jim Goodnow II
jim at thegoodnows.net
Tue Nov 2 14:25:23 PDT 2010
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.
More information about the cfe-commits