[PATCH] Sema: Eliminate recursion from checkForFunctionCall()

[Duncan P. N. Exon Smith]

> On 2015-Jul-23, at 08:57, Vedant Kumar <vsk at apple.com> wrote:
> 
> Ok! The NFC preparatory patch is in part1, and the behavior-changing work is in part2.
> 
> No problems with check-clang.
> 
> <checkForFunctionCall-part1.patch><checkForFunctionCall-part2.patch>

Almost there.  A bunch more comments on the first patch, but some can
be left for follow-ups.  If you fix the function name and flatten the
`MCE` block I'll commit for you.

> diff --git a/lib/Sema/AnalysisBasedWarnings.cpp b/lib/Sema/AnalysisBasedWarnings.cpp
> index f2ff48a..ff21076 100644
> --- a/lib/Sema/AnalysisBasedWarnings.cpp
> +++ b/lib/Sema/AnalysisBasedWarnings.cpp
> @@ -171,6 +171,48 @@ enum RecursiveState {
>    FoundPathWithNoRecursiveCall
>  };
>  
> +static RecursiveState nextRecursiveStateInPath(const FunctionDecl *FD,

LLVM coding style prescribes verbs for function names.

Maybe, `getNextRecursiveStateInPath()`?  `getNextRecursiveState()`?

> +                                               CFGBlock &Block,
> +                                               RecursiveState State) {
> +  if (State != FoundPathWithNoRecursiveCall)
> +    return State;

Can you just check this before calling the function?  If so, you don't
need to pass `State` in at all.

Actually, this whole function looks like it's just checking for a
recursive path.  You could change it to return `bool`, and rename it
something like `hasRecursiveCall()`, and leave the state-related logic
out of the helper.  Maybe better in a follow-up patch?

> +
> +  // If the current state is FoundPathWithNoRecursiveCall, the successors
> +  // will be either FoundPathWithNoRecursiveCall or FoundPath.  To determine
> +  // which, process all the Stmt's in this block to find any recursive calls.
> +  for (const auto &B : Block) {
> +    if (B.getKind() != CFGElement::Statement)
> +      continue;
> +
> +    const CallExpr *CE = dyn_cast<CallExpr>(B.getAs<CFGStmt>()->getStmt());
> +    if (!CE || !CE->getCalleeDecl() ||
> +        CE->getCalleeDecl()->getCanonicalDecl() != FD)
> +      continue;
> +
> +    // Skip function calls which are qualified with a templated class.
> +    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;
> +        }
> +      }
> +    }
> +
> +    if (const CXXMemberCallExpr *MCE = dyn_cast<CXXMemberCallExpr>(CE)) {
> +      if (isa<CXXThisExpr>(MCE->getImplicitObjectArgument()) ||
> +          !MCE->getMethodDecl()->isVirtual()) {
> +        return FoundPath;
> +      }
> +    } else {
> +      return FoundPath;
> +    }

I think this would be clearer like this:

    const CXXMemberCallExpr *MCE = dyn_cast<CXXMemberCallExpr>(CE);
    if (!MCE || isa<CXXThisExpr>(MCE->getImplicitObjectArgument()) ||
        !MCE->getMethodDecl()->isVirtual())
      return FoundPath;

> +  }
>  
> +  return State;

This is always `FoundPathWithNoRecursiveCall`, right?

> +}
> +
>  static void checkForFunctionCall(Sema &S, const FunctionDecl *FD,
>                                   CFGBlock &Block, unsigned ExitID,
>                                   llvm::SmallVectorImpl<RecursiveState> &States,
> @@ -187,42 +229,7 @@ static void checkForFunctionCall(Sema &S, const FunctionDecl *FD,
>    if (ID == ExitID && State == FoundPathWithNoRecursiveCall)
>      return;
>  
> -  if (State == FoundPathWithNoRecursiveCall) {
> -    // If the current state is FoundPathWithNoRecursiveCall, the successors
> -    // will be either FoundPathWithNoRecursiveCall or FoundPath.  To determine
> -    // which, process all the Stmt's in this block to find any recursive calls.
> -    for (const auto &B : Block) {
> -      if (B.getKind() != CFGElement::Statement)
> -        continue;
> -
> -      const CallExpr *CE = dyn_cast<CallExpr>(B.getAs<CFGStmt>()->getStmt());
> -      if (CE && CE->getCalleeDecl() &&
> -          CE->getCalleeDecl()->getCanonicalDecl() == FD) {
> -
> -        // Skip function calls which are qualified with a templated class.
> -        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;
> -            }
> -          }
> -        }
> -
> -        if (const CXXMemberCallExpr *MCE = dyn_cast<CXXMemberCallExpr>(CE)) {
> -          if (isa<CXXThisExpr>(MCE->getImplicitObjectArgument()) ||
> -              !MCE->getMethodDecl()->isVirtual()) {
> -            State = FoundPath;
> -            break;
> -          }
> -        } else {
> -          State = FoundPath;
> -          break;
> -        }
> -      }
> -    }
> -  }
> +  State = nextRecursiveStateInPath(FD, Block, State);

For clarity: what I'm suggesting above is to change this call to:

    if (State == FoundPathWithNoRecursiveCall)
      State = getNextRecursiveState(FD, Block);

or, even better:

    if (State == FoundPathWithNoRecursiveCall)
      if (hasRecursiveCallInPath(FD, Block))
        State = FoundPath;

In either case, this could be merged with the early exit:

    if (State == FoundPathWithNoRecursiveCall)
      if (ExitID == ID)
        return;
      if (hasRecusiveCallInPath(FD, Block)
        State = FoundPath;
    }

These might be better left to a follow-up though.

>  
>    for (CFGBlock::succ_iterator I = Block.succ_begin(), E = Block.succ_end();
>         I != E; ++I)
>