[cfe-commits] r164339 - in /cfe/trunk: include/clang/Analysis/ include/clang/StaticAnalyzer/Core/ include/clang/StaticAnalyzer/Core/BugReporter/ include/clang/StaticAnalyzer/Core/PathSensitive/ lib/Analysis/ lib/StaticAnalyzer/Core/ test/Analysis/

[Anna Zaks]

On Sep 21, 2012, at 9:24 AM, Jordan Rose <jordan_rose at apple.com> wrote:

> 
> On Sep 20, 2012, at 23:00 , Ted Kremenek <kremenek at apple.com> wrote:
> 
>>> 
>>> 
>>>> }
>>>> 
>>>> AnalysisDeclContext *AnalysisDeclContextManager::getContext(const Decl *D) {
>>>> +  if (const FunctionDecl *FD = dyn_cast<FunctionDecl>(D)) {
>>>> +    FD->hasBody(FD);

>>>> +    D = FD;

I think FD->hasBody(D); + a comment will make this more readable.


>>>> +  }
>>> 
>>> So there is apparently a Decl::getBody that does the right thing (i.e. returns 0 if there is no body). Can we just use that?
>> 
>> We're not trying to get the body here.  We're trying to update the D to be the one that has the body (if one exists), and use that as the Decl* we use to construct the AnalysisDeclContext.  Maybe getBody() looks cleaner, but it functionality isn't really any different.
> 
> Oops, right. What I really want is getDefinition, which currently doesn't exist, but easily could.
> 
> 
>> Note that apparently not updating 'D' here correctly broke a ton of stuff.
> 
> Yes. Yes it does.
> 
> 
> 
>>> 
>>>> +  // Everything checks out.  Create a fake body that just calls the block.
>>>> +  // This is basically just an AST dump of:
>>>> +  //
>>>> +  // void dispatch_sync(dispatch_queue_t queue, void (^block)(void)) {
>>>> +  //   block();
>>>> +  // }
>>>> +  //
>>>> +  DeclRefExpr *DR = DeclRefExpr::CreateEmpty(C, false, false, false, false);
>>>> +  DR->setDecl(const_cast<ParmVarDecl*>(PV));
>>>> +  DR->setValueKind(VK_LValue);
>>>> +  ImplicitCastExpr *ICE = ImplicitCastExpr::Create(C, Ty, CK_LValueToRValue,
>>>> +                                                   DR, 0, VK_RValue);
>>>> +  CallExpr *CE = new (C) CallExpr(C, ICE, ArrayRef<Expr*>(), C.VoidTy,
>>>> +                                  VK_RValue, SourceLocation());
>>>> +  return CE;
>>>> +}
>>> 
>>> The name for ArrayRef<Expr*>() is 
>> 
>> Hmm?
> 
> Oops. I meant to go look up a nice alias because the template is ugly, and in fact MultiExprArg is compatible (it's MutableArrayRef<Expr*>), but this does match the form in CallExpr's constructor.
> 
> 
>>> It is probably not a good idea to make a function body that isn't a CompoundStmt or CXXTryStmt.
>> 
>> Why?  I know how the CFGBuilder works.  This isn't an issue.  The CFGBuilder just flattens out the CompoundStmt.  That concept  doesn't even exist in the CFG.
>> 
>>> Right now I don't think anything depends on it besides the diagnostics, which don't apply here, but still.
> 
> I guess if it's ONLY diagnostics that depend on it, it doesn't matter. (PathDiagnosticLocation::createDeclBegin has a FIXME about 'try'.)
> 
> 
> 
>> 
>>> 
>>> 
>>>>  switch (piece->getKind()) {
>>>>    case PathDiagnosticPiece::Call: {
>>>>      PathDiagnosticCallPiece *call = cast<PathDiagnosticCallPiece>(piece);
>>>> @@ -142,8 +148,17 @@
>>>>      }
>>>>      // Recursively clean out the subclass.  Keep this call around if
>>>>      // it contains any informative diagnostics.
>>>> -        if (!RemoveUneededCalls(call->path, R))
>>>> +        PathDiagnosticCallPiece *NewCallWithLoc =
>>>> +          call->getLocation().asLocation().isValid()
>>>> +            ? call : CallWithLoc;
>>> 
>>> I think there should actually be an assertion here -- only inlined functions can be synthesized, so there will always be a non-synthesized call with a valid location.
>> 
>> Not true.  The synthesized function itself can call other functions.  That call doesn't have a valid location.
> 
> Right. I meant that if !call->getLocation().asLocation().isValid(), assert CallWithLoc. Or rather, just assert NewCallWithLoc after this, and then remove one of the tests from the 'if' below...
> 
> 
>>> 
>>>> +        if (!RemoveUneededCalls(call->path, R, NewCallWithLoc))
>>>>        continue;
>>>> +
>>>> +        if (NewCallWithLoc == CallWithLoc && CallWithLoc) {
>>>> +          call->callEnter = CallWithLoc->callEnter;
>>>> +        }
>>> 
>>> Extra braces? Also, when does callExit get set?
>> 
>> Good point.  We'll need a way to propagate back the call exit location from the bottom.
> 
> Well, in the normal traversal, the exit information gets set first. In RemoveUneededCalls (hm, we have a typo there), we already have both entry and exit information, so we should be able to just set it here as well.
> 
> 
> 
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits