[PATCH] D34202: [clang-tidy] readability-function-size: fix nesting level calculation

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri created this revision.
lebedev.ri added a project: clang-tools-extra.

A followup for https://reviews.llvm.org/D32942.

Malcolm Parsons has provided a valid testcase that the initial version of the check complained about nested `if`'s.
As it turns out, the culprit is the **partially** un-intentional `switch` fallthrough.
So fix the unintentional part of the fallthrough, and add testcases with nested `if`' where there should be a warning and shouldn't be a warning.

I guess, now it would be actually possible to pick some reasonable default for `NestingThreshold` setting.


Repository:
  rL LLVM

https://reviews.llvm.org/D34202

Files:
  clang-tidy/readability/FunctionSizeCheck.cpp
  test/clang-tidy/readability-function-size.cpp


Index: test/clang-tidy/readability-function-size.cpp
===================================================================
--- test/clang-tidy/readability-function-size.cpp
+++ test/clang-tidy/readability-function-size.cpp
@@ -63,8 +63,9 @@
 #define macro() {int x; {int y; {int z;}}}
 
 void baz0() { // 1
-// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: function 'baz0' exceeds recommended size/complexity
-// CHECK-MESSAGES: :[[@LINE-2]]:6: note: 9 statements (threshold 0)
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: function 'baz0' exceeds recommended size/complexity
+  // CHECK-MESSAGES: :[[@LINE-2]]:6: note: 27 lines including whitespace and comments (threshold 0)
+  // CHECK-MESSAGES: :[[@LINE-3]]:6: note: 9 statements (threshold 0)
   int a;
   { // 2
     int b;
@@ -87,5 +88,51 @@
   }
   macro()
 // CHECK-MESSAGES: :[[@LINE-1]]:3: note: nesting level 3 starts here (threshold 2)
-// CHECK-MESSAGES: :[[@LINE-27]]:25: note: expanded from macro 'macro'
+// CHECK-MESSAGES: :[[@LINE-28]]:25: note: expanded from macro 'macro'
+}
+
+// check that nested if's are not reported. this was broken initially
+void nesting_if() { // 1
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: function 'nesting_if' exceeds recommended size/complexity
+  // CHECK-MESSAGES: :[[@LINE-2]]:6: note: 18 lines including whitespace and comments (threshold 0)
+  // CHECK-MESSAGES: :[[@LINE-3]]:6: note: 15 statements (threshold 0)
+  // CHECK-MESSAGES: :[[@LINE-4]]:6: note: 5 branches (threshold 0)
+  if (true) { // 2
+     int j;
+  } else if (true) { // 2
+     int j;
+     if (true) { // 3
+       // CHECK-MESSAGES: :[[@LINE-1]]:16: note: nesting level 3 starts here (threshold 2)
+       int j;
+     }
+  } else if (true) { // 2
+     int j;
+  } else if (true) { // 2
+     int j;
+  }
+}
+
+// however this should warn
+void bad_if_nesting() { // 1
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: function 'bad_if_nesting' exceeds recommended size/complexity
+// CHECK-MESSAGES: :[[@LINE-2]]:6: note: 22 lines including whitespace and comments (threshold 0)
+// CHECK-MESSAGES: :[[@LINE-3]]:6: note: 12 statements (threshold 0)
+// CHECK-MESSAGES: :[[@LINE-4]]:6: note: 4 branches (threshold 0)
+  if (true) {    // 2
+    int j;
+  } else { // 2
+    if (true) { // 3
+      // CHECK-MESSAGES: :[[@LINE-1]]:15: note: nesting level 3 starts here (threshold 2)
+      int j;
+    } else { // 3
+      // CHECK-MESSAGES: :[[@LINE-1]]:12: note: nesting level 3 starts here (threshold 2)
+      if (true) { // 4
+        int j;
+      } else { // 4
+        if (true) { // 5
+          int j;
+        }
+      }
+    }
+  }
 }
Index: clang-tidy/readability/FunctionSizeCheck.cpp
===================================================================
--- clang-tidy/readability/FunctionSizeCheck.cpp
+++ clang-tidy/readability/FunctionSizeCheck.cpp
@@ -36,15 +36,18 @@
     case Stmt::ForStmtClass:
     case Stmt::SwitchStmtClass:
       ++Info.Branches;
-    // fallthrough
+      LLVM_FALLTHROUGH;
     case Stmt::CompoundStmtClass:
-      // If this new compound statement is located in a compound statement,
-      // which is already nested NestingThreshold levels deep, record the start
-      // location of this new compound statement
-      if (CurrentNestingLevel == Info.NestingThreshold)
-        Info.NestingThresholders.push_back(Node->getLocStart());
+      if(Node->getStmtClass() == Stmt::CompoundStmtClass) {
+        // If this new compound statement is located in a compound statement,
+        // which is already nested NestingThreshold levels deep, record the start
+        // location of this new compound statement
+        if (CurrentNestingLevel == Info.NestingThreshold)
+          Info.NestingThresholders.push_back(Node->getLocStart());
+
+        ++CurrentNestingLevel;
+      }
 
-      ++CurrentNestingLevel;
       TrackedParent.push_back(true);
       break;
     default:


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D34202.102526.patch
Type: text/x-patch
Size: 3897 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20170614/a0125ee5/attachment.bin>