[cfe-commits] r167351 - in /cfe/trunk: include/clang/Analysis/AnalysisContext.h include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h lib/Analysis/AnalysisDeclContext.cpp lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp

[Anna Zaks]

On Nov 3, 2012, at 10:38 AM, Jordan Rose <jordan_rose at apple.com> wrote:

> 
> On Nov 2, 2012, at 22:18 , Anna Zaks <ganna at apple.com> wrote:
> 
>> 
>> On Nov 2, 2012, at 8:45 PM, Jordan Rose wrote:
>> 
>>> This is silly.
>> What is silly?
> 
> Both LocationContext and CheckerContext now have two methods which are opposites (i.e. one returns !other), but they are implemented in different ways.

This is not the case. 

LocationContext does not have isInlined method at all. That is the main contribution of the commit.

The isInlined member variable in the CheckerContext has a different meaning than isInTopFrame. It is directly set in the ExprEngine when the checker callback is called. Even though isInTopFrame and isInlined are opposite in the current implementation, one is generally a subset of the other. So I do not see any problem exposing both an letting the user of the API decide which one they need to use.

Anna.

>> 
>>> For CheckerContext, inTopFrame is !isInInlined.
>>> For LocationContext, this shouldn't be virtual; it's just getEnclosingStackFrame()->getParent() == 0.
>> 
>> I don't see the advantage of this. When you ask if a StackFrameContext is inTopFrame, we would have to call getEnclosingStackFrame instead of just evaluating "Parent==0".
> 
> Okay, fair enough.  I'm not sure whether the virtual call is going to be faster or slower than the regular call plus an extra dyn_cast, and I suppose I was assuming "slower".
> 
> Anyway, even if the LocationContext method stands, CheckerContext should be updated to call through to it for both accessors, or one of the accessors should be removed.
> 
> 
> 
>> Anna.
>> 
>>> 
>>> 
>>> On Nov 2, 2012, at 19:54 , Anna Zaks <ganna at apple.com> wrote:
>>> 
>>>> Author: zaks
>>>> Date: Fri Nov  2 21:54:16 2012
>>>> New Revision: 167351
>>>> 
>>>> URL: http://llvm.org/viewvc/llvm-project?rev=167351&view=rev
>>>> Log:
>>>> [analyzer] add LocationContext::inTopFrame() helper.
>>>> 
>>>> Modified:
>>>> cfe/trunk/include/clang/Analysis/AnalysisContext.h
>>>> cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h
>>>> cfe/trunk/lib/Analysis/AnalysisDeclContext.cpp
>>>> cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
>>>> 
>>>> Modified: cfe/trunk/include/clang/Analysis/AnalysisContext.h
>>>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Analysis/AnalysisContext.h?rev=167351&r1=167350&r2=167351&view=diff
>>>> ==============================================================================
>>>> --- cfe/trunk/include/clang/Analysis/AnalysisContext.h (original)
>>>> +++ cfe/trunk/include/clang/Analysis/AnalysisContext.h Fri Nov  2 21:54:16 2012
>>>> @@ -237,6 +237,9 @@
>>>> 
>>>> const StackFrameContext *getCurrentStackFrame() const;
>>>> 
>>>> +  /// Return true if the current LocationContext has no caller context.
>>>> +  virtual bool inTopFrame() const;
>>>> +
>>>> virtual void Profile(llvm::FoldingSetNodeID &ID) = 0;
>>>> 
>>>> public:
>>>> @@ -271,6 +274,9 @@
>>>> 
>>>> const CFGBlock *getCallSiteBlock() const { return Block; }
>>>> 
>>>> +  /// Return true if the current LocationContext has no caller context.
>>>> +  virtual bool inTopFrame() const { return getParent() == 0;  }
>>>> +
>>>> unsigned getIndex() const { return Index; }
>>>> 
>>>> void Profile(llvm::FoldingSetNodeID &ID);
>>>> 
>>>> Modified: cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h
>>>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h?rev=167351&r1=167350&r2=167351&view=diff
>>>> ==============================================================================
>>>> --- cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h (original)
>>>> +++ cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h Fri Nov  2 21:54:16 2012
>>>> @@ -100,6 +100,9 @@
>>>>  return Pred->getStackFrame();
>>>> }
>>>> 
>>>> +  /// Return true if the current LocationContext has no caller context.
>>>> +  bool inTopFrame() const { return getLocationContext()->inTopFrame();  }
>>>> +
>>>> /// Returns true if the predecessor is within an inlined function/method.
>>>> bool isWithinInlined() {
>>>>  return (getStackFrame()->getParent() != 0);
>>>> 
>>>> Modified: cfe/trunk/lib/Analysis/AnalysisDeclContext.cpp
>>>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Analysis/AnalysisDeclContext.cpp?rev=167351&r1=167350&r2=167351&view=diff
>>>> ==============================================================================
>>>> --- cfe/trunk/lib/Analysis/AnalysisDeclContext.cpp (original)
>>>> +++ cfe/trunk/lib/Analysis/AnalysisDeclContext.cpp Fri Nov  2 21:54:16 2012
>>>> @@ -355,6 +355,10 @@
>>>> return NULL;
>>>> }
>>>> 
>>>> +bool LocationContext::inTopFrame() const {
>>>> +  return getCurrentStackFrame()->inTopFrame();
>>>> +}
>>>> +
>>>> bool LocationContext::isParentOf(const LocationContext *LC) const {
>>>> do {
>>>>  const LocationContext *Parent = LC->getParent();
>>>> 
>>>> Modified: cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
>>>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp?rev=167351&r1=167350&r2=167351&view=diff
>>>> ==============================================================================
>>>> --- cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp (original)
>>>> +++ cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp Fri Nov  2 21:54:16 2012
>>>> @@ -3190,12 +3190,6 @@
>>>> // Handle return statements.
>>>> //===----------------------------------------------------------------------===//
>>>> 
>>>> -// Return true if the current LocationContext has no caller context.
>>>> -static bool inTopFrame(CheckerContext &C) {
>>>> -  const LocationContext *LC = C.getLocationContext();
>>>> -  return LC->getParent() == 0;  
>>>> -}
>>>> -
>>>> void RetainCountChecker::checkPreStmt(const ReturnStmt *S,
>>>>                                    CheckerContext &C) const {
>>>> 
>>>> @@ -3204,7 +3198,7 @@
>>>> // better checking even for inlined calls, and see if they match
>>>> // with their expected semantics (e.g., the method should return a retained
>>>> // object, etc.).
>>>> -  if (!inTopFrame(C))
>>>> +  if (!C.inTopFrame())
>>>>  return;
>>>> 
>>>> const Expr *RetE = S->getRetValue();
>>>> 
>>>> 
>>>> _______________________________________________
>>>> cfe-commits mailing list
>>>> cfe-commits at cs.uiuc.edu
>>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>> 
>> 
>