[PATCH] Sema: Eliminate recursion from checkForFunctionCall()

[Vedant Kumar]

Ok! The NFC preparatory patch is in part1, and the behavior-changing work is in part2.

No problems with check-clang.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: checkForFunctionCall-part1.patch
Type: application/octet-stream
Size: 3741 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150723/0a542d98/attachment.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: checkForFunctionCall-part2.patch
Type: application/octet-stream
Size: 1890 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150723/0a542d98/attachment-0001.obj>
-------------- next part --------------


> On Jul 22, 2015, at 4:22 PM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
> 
> 
>> 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)
>> 
>