[PATCH] D43737: Improve -Winfinite-recursion

[Richard Trieu via Phabricator via cfe-commits]

rtrieu added a comment.

>   I believe you were around this code last, so can you remember why it was there?

Yes, that's an early exit to speed up the check.  You can remove that check and add a test case for it.

This was a little confusing for me, so let's refactor it a little.  These changes will make it so that any CFGBlock in the WorkList has already been checked, so it can go straight to checking the successors.



================
Comment at: lib/Sema/AnalysisBasedWarnings.cpp:203-204
 
-// All blocks are in one of three states.  States are ordered so that blocks
-// can only move to higher states.
-enum RecursiveState {
-  FoundNoPath,
-  FoundPath,
-  FoundPathWithNoRecursiveCall
-};
-
 // Returns true if there exists a path to the exit block and every path
 // to the exit block passes through a call to FD.
 static bool checkForRecursiveFunctionCall(const FunctionDecl *FD, CFG *cfg) {
----------------
Update comment to reflect your changes.


================
Comment at: lib/Sema/AnalysisBasedWarnings.cpp:224-225
 
-  // Mark all nodes as FoundNoPath, then set the status of the entry block.
-  SmallVector<RecursiveState, 16> States(cfg->getNumBlockIDs(), FoundNoPath);
-  States[cfg->getEntry().getBlockID()] = FoundPathWithNoRecursiveCall;
-
-  // Make the processing stack and seed it with the entry block.
-  SmallVector<CFGBlock *, 16> Stack;
-  Stack.push_back(&cfg->getEntry());
-
-  while (!Stack.empty()) {
-    CFGBlock *CurBlock = Stack.back();
-    Stack.pop_back();
+  // Seed the work list with the entry block.
+  foundRecursion |= analyzeSuccessor(&cfg->getEntry());
 
----------------
The entry CFGBlock is special.  It will never trigger on any of the conditions we are checking for.  Just push it into WorkList and Visited.  It has no statements, so it can't have a recursive call.

http://clang.llvm.org/docs/InternalsManual.html#entry-and-exit-blocks

Technically, it has no incoming edges, so you don't even need to insert it into Visited.


================
Comment at: lib/Sema/AnalysisBasedWarnings.cpp:232-233
+    // Found a path to the exit node without a recursive call.
+    if (ExitID == ID)
+      return false;
 
----------------
ID is not longer needed as an index so it is only needed here, so it should be inlined.

Also, move this check in the loop over the successors, so there are no checks on the current CFG block.  This way, all checks happen at the same place.


================
Comment at: lib/Sema/AnalysisBasedWarnings.cpp:237
+      if (*I)
+        foundRecursion |= analyzeSuccessor(*I);
   }
----------------
Since the lambda isn't needed for seeding the WorkList, this is the only use and it should be inlined here.

Also, use:
```
   if (cond)
     foundRecursion = true;
```
instead of:

```
  foundRecursion |= true;
```


https://reviews.llvm.org/D43737