[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

[Jordan Rose]

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".


> +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.


> +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.



> @@ -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).