[cfe-commits] r160530 - in /cfe/trunk: include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h lib/StaticAnalyzer/Core/CheckerManager.cpp lib/StaticAnalyzer/Core/ExprEngineCXX.cpp lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp lib/StaticAnalyzer/Core/ExprEngineObjC.cpp

[Anna Zaks]

Thanks Jordan!

On Jul 19, 2012, at 5:21 PM, Jordan Rose wrote:

> It came out very nicely!
> 
> A few small comments...
> 
> On Jul 19, 2012, at 4:38 PM, Anna Zaks wrote:
> 
>> +
>> +  /// \bried Default implementation of call evaluation.
>> +  void defaultEvalCall(NodeBuilder &B, ExplodedNode *Pred,
>>                       const CallEvent &Call);
> 
> Typo: "bried".
> 
thanks
> 
>> +ProgramStateRef ExprEngine::bindReturnValue(const CallEvent &Call,
>> +                                            const LocationContext *LCtx,
>> +                                            ProgramStateRef State) {
>> +  const Expr *E = Call.getOriginExpr();
>> +  if (!E)
>> +    return State;
>> +
>> +  // Some method families have known return values.
>> +  if (const ObjCMethodCall *Msg = dyn_cast<ObjCMethodCall>(&Call)) {
>> +    switch (Msg->getMethodFamily()) {
>> +    default:
>> +      break;
>> +    case OMF_autorelease:
>> +    case OMF_retain:
>> +    case OMF_self: {
>> +      // These methods return their receivers.
>> +      return State->BindExpr(E, LCtx, Msg->getReceiverSVal());
>> +      break;
>> +    }
>> +    }
>> +  }
> 
> The inner braces are not necessary anymore here. Also, you have a break after a return.

I'd like to keep the braces of the if since the switch is visually long (do we have a coding standard for unnecessary braces?).

> 
> 
>> +void ExprEngine::defaultEvalCall(NodeBuilder &Bldr, ExplodedNode *Pred,
>>                                 const CallEvent &Call) {
>> +  ProgramStateRef State = 0;
>> +  const Expr *E = Call.getOriginExpr();
>> +
>>  // Try to inline the call.
>>  // The origin expression here is just used as a kind of checksum;
>>  // for CallEvents that do not have origin expressions, this should still be
>>  // safe.
>> -  const Expr *E = Call.getOriginExpr();
>> -  ProgramStateRef state = getInlineFailedState(Pred, E);
>> -  if (state == 0 && inlineCall(Dst, Call, Pred))
>> -    return;
>> +  if (!isa<ObjCMethodCall>(Call)) {
>> +    State = getInlineFailedState(Pred->getState(), E);
>> +    if (State == 0 && inlineCall(Call, Pred)) {
>> +      // If we inlined the call, the successor has been manually added onto
>> +      // the work list and we should not consider it for subsequent call
>> +      // handling steps.
>> +      Bldr.takeNodes(Pred);
>> +      return;
>> +    }
>> +  }
>> 
> 
> I would just take out the check for isa<ObjCMethodCall>. The check for "dynamic dispatch" inside inlineCall should catch this, and getInlineFailedState isn't too expensive.
> 
Yes, this is WIP. Plan is to remove it after I figure out if inlineCall can handle this (which is probably does according to your comment).
> 
> 
>> @@ -182,13 +180,19 @@
>> 
>>        // Check if the "raise" message was sent.
>>        assert(notNilState);
>> -        if (msg.getSelector() == RaiseSel)
>> -          RaisesException = true;
>> +        if (msg.getSelector() == RaiseSel) {
>> +          // If we raise an exception, for now treat it as a sink.
>> +          // Eventually we will want to handle exceptions properly.
>> +          Bldr.generateNode(currentStmt, Pred, Pred->getState(), true);
>> +          continue;
>> +        }
>> 
>> -        // If we raise an exception, for now treat it as a sink.
>> -        // Eventually we will want to handle exceptions properly.
>> -        // Dispatch to plug-in transfer function.
>> -        evalObjCMessage(Bldr, msg, Pred, notNilState, RaisesException);
>> +        // Generate a transition to non-Nil state.
>> +        if (notNilState != state)
>> +          Pred = Bldr.generateNode(currentStmt, Pred, notNilState);
>> +
>> +        // Evaluate the call.
>> +        defaultEvalCall(Bldr, Pred, msg);
>>      }
>>    } else {
>>      // Check for special class methods.
>> @@ -222,19 +226,25 @@
>>          }
>> 
>>          Selector S = msg.getSelector();
>> +          bool RaisesException = false;
>>          for (unsigned i = 0; i < NUM_RAISE_SELECTORS; ++i) {
>>            if (S == NSExceptionInstanceRaiseSelectors[i]) {
>>              RaisesException = true;
>>              break;
>>            }
>>          }
>> +          if (RaisesException) {
>> +            // If we raise an exception, for now treat it as a sink.
>> +            // Eventually we will want to handle exceptions properly.
>> +            Bldr.generateNode(currentStmt, Pred, Pred->getState(), true);
>> +            continue;
>> +          }
>> +
>>        }
>>      }
>> 
>> -      // If we raise an exception, for now treat it as a sink.
>> -      // Eventually we will want to handle exceptions properly.
>> -      // Dispatch to plug-in transfer function.
>> -      evalObjCMessage(Bldr, msg, Pred, Pred->getState(), RaisesException);
>> +      // Evaluate the call.
>> +      defaultEvalCall(Bldr, Pred, msg);
>>    }
>>  }
> 
> You can probably merge some pieces of the two branches now (the class and instance method branches).
I'll move out the call to defaultEvalCall, bit not much else is easily movable.