[PATCH] Sema: Eliminate recursion from checkForFunctionCall()

[Duncan P. N. Exon Smith]

> On 2015-Jul-22, at 14:31, Vedant Kumar <vsk at apple.com> wrote:
> 
>> This is a fairly large block of code to further indent.  Can you
>> move this to a helper function as a separate, preparatory patch
>> with no functionality change (NFC) that returns the new value of
>> `State`?
> 
> I’ve attached a patch which separates the state update logic in checkForFunctionCall into a helper routine.
> 
> I’ll send a follow-up patch with the rest of the changes you requested shortly.
> 
> 
> <checkForFunctionCall-1.patch>
> 


> diff --git a/lib/Sema/AnalysisBasedWarnings.cpp b/lib/Sema/AnalysisBasedWarnings.cpp
> index f2ff48a..60bf5a0 100644
> --- a/lib/Sema/AnalysisBasedWarnings.cpp
> +++ b/lib/Sema/AnalysisBasedWarnings.cpp
> @@ -171,21 +171,14 @@ enum RecursiveState {
>    FoundPathWithNoRecursiveCall
>  };
>  
> -static void checkForFunctionCall(Sema &S, const FunctionDecl *FD,
> -                                 CFGBlock &Block, unsigned ExitID,
> -                                 llvm::SmallVectorImpl<RecursiveState> &States,
> -                                 RecursiveState State) {
> +static RecursiveState updateRecursiveState(const FunctionDecl *FD,

Sounds like it mutates an inout parameter, and the name doesn't describe
the context.  Maybe `computeRecursiveStateForSuccessors()`?  That's
rather long, so feel free to come up with something better.

> +                                           CFGBlock &Block, unsigned ExitID,
> +                                           RecursiveState State) {
>    unsigned ID = Block.getBlockID();
>  
> -  // A block's state can only move to a higher state.
> -  if (States[ID] >= State)
> -    return;
> -
> -  States[ID] = State;
> -
>    // Found a path to the exit node without a recursive call.
>    if (ID == ExitID && State == FoundPathWithNoRecursiveCall)
> -    return;
> +    return State;

I think you need to leave this condition in `checkForFunctionCall()`,
since otherwise you're losing the early return.

With that out of the function, you don't need `ExitID` here at all.

>  
>    if (State == FoundPathWithNoRecursiveCall) {

Can you convert this to an early return?

    if (State != FoundPathWithNoRecursiveCall)
      return State;

Alternatively, since this is right at the top of the function, maybe you
shouldn't call this helper at all if it's not in the right state?  Then
you don't need to pass `State` in at all, just assume that it's
`FoundPathWithNoRecursiveCall`.

>      // If the current state is FoundPathWithNoRecursiveCall, the successors
> @@ -200,12 +193,12 @@ static void checkForFunctionCall(Sema &S, const FunctionDecl *FD,
>            CE->getCalleeDecl()->getCanonicalDecl() == FD) {

Can you reduce nesting here?  I'm thinking something like:

-    const CallExpr *CE = dyn_cast<CallExpr>(B.getAs<CFGStmt>()->getStmt());
-    if (CE && CE->getCalleeDecl() &&
-        CE->getCalleeDecl()->getCanonicalDecl() == FD) {
+    const CallExpr *CE = dyn_cast<CallExpr>(B.getAs<CFGStmt>()->getStmt());
+    if (!CE || !CE->getCalleeDecl() ||
+        CE->getCalleeDecl()->getCanonicalDecl() != FD)
+      continue;

would clean things up a fair bit.

>  
>          // Skip function calls which are qualified with a templated class.
> -        if (const DeclRefExpr *DRE = dyn_cast<DeclRefExpr>(
> -                CE->getCallee()->IgnoreParenImpCasts())) {
> +        if (const DeclRefExpr *DRE =
> +                dyn_cast<DeclRefExpr>(CE->getCallee()->IgnoreParenImpCasts())) {
>            if (NestedNameSpecifier *NNS = DRE->getQualifier()) {
>              if (NNS->getKind() == NestedNameSpecifier::TypeSpec &&
>                  isa<TemplateSpecializationType>(NNS->getAsType())) {
> -               continue;
> +              continue;
>              }
>            }
>          }

Please clean up the rest of the new helper function as well.  There are
some `break`s that should just be `return`s, etc.

> @@ -224,6 +217,23 @@ static void checkForFunctionCall(Sema &S, const FunctionDecl *FD,
>      }
>    }
>  
> +  return State;
> +}
> +
> +static void checkForFunctionCall(Sema &S, const FunctionDecl *FD,
> +                                 CFGBlock &Block, unsigned ExitID,
> +                                 llvm::SmallVectorImpl<RecursiveState> &States,
> +                                 RecursiveState State) {
> +  unsigned ID = Block.getBlockID();
> +
> +  // A block's state can only move to a higher state.
> +  if (States[ID] >= State)
> +    return;
> +
> +  States[ID] = State;

(This is where the return for `ID == ExitID` is missing.)

> +
> +  State = updateRecursiveState(FD, Block, ExitID, State);
> +
>    for (CFGBlock::succ_iterator I = Block.succ_begin(), E = Block.succ_end();
>         I != E; ++I)
>      if (*I)
>