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

[lebedev.ri]

Summary

A followup for 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 rewrite the NestingThreshold logic without ab-using+mis-using that switch with fallthrough, and add testcases with nested if' where there should be a warning and shouldn't be a warning. This results in a cleaner, simpler code, too.

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

Fixes PR33454.

[lebedev.ri]

Hmm, this is moving in the right direction, but now it invalidated (decreases the nesting level) too eagerly

[lebedev.ri]

Ok, i really messed up the initial D32942 :)
Malcolm, thank you for bothering to report :)
This should be so much better.

[malcolm.parsons]

clang-tidy/readability/FunctionSizeCheck.cpp

38 ↗

(On Diff #102533)

I don't think this assert adds anything.

62 ↗

(On Diff #102533)

Comments are sentences, so start with a capital letter.

[lebedev.ri]

Fix spelling.

clang-tidy/readability/FunctionSizeCheck.cpp

38 ↗

(On Diff #102533)

Yes, however the original if did have that check.
I think, having an assert here may help to not break this in the future.

[lebedev.ri]

Simplify it even further by moving the logic into it's proper place - TraverseCompoundStmt()

[malcolm.parsons]

What do you expect for this?

if (true)
    if (true)
        if (true) {
            int j;
        }

[lebedev.ri]

In D34202#780394, @malcolm.parsons wrote:

What do you expect for this?

if (true)
    if (true)
        if (true) {
            int j;
        }

that it is equivalent to

if (true && true && true) { // 1
  int j;
}

This was the intent of that option. There is only one compound statement in your example. All the docs say that it counts compound statements
https://github.com/llvm-mirror/clang-tools-extra/blob/9fd3636de8d7034ca4640939fefebd9833ef9ea0/docs/clang-tidy/checks/readability-function-size.rst

.. option:: NestingThreshold

    Flag compound statements ...

[malcolm.parsons]

My coding standards require the {}, so while I may not agree with your definition of nesting, it doesn't matter.

[lebedev.ri]

In D34202#780424, @malcolm.parsons wrote:

My coding standards require the {}, so while I may not agree with your definition of nesting, it doesn't matter.

Well, seeing readability-deep-nesting.cpp in the issue, i suppose the definition might be adjusted to fit that too, to avoid having more than one check doing just slightly different checking...

[malcolm.parsons]

LGTM!

clang-tidy/readability/FunctionSizeCheck.cpp

58 ↗

(On Diff #102548)

Missing full stop.

clang-tools-extra/trunk/clang-tidy/readability/FunctionSizeCheck.cpp

bool TraverseStmt(Stmt *Node) {
     switch (Node->getStmtClass()) {
     case Stmt::IfStmtClass:
     case Stmt::WhileStmtClass:
     case Stmt::DoStmtClass:
     case Stmt::CXXForRangeStmtClass:
     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());
-
-      ++CurrentNestingLevel;
       TrackedParent.push_back(true);
       break;
     default:
       TrackedParent.push_back(false);
       break;
     }
 
     Base::TraverseStmt(Node);
 
-    if (TrackedParent.back())
-      --CurrentNestingLevel;
     TrackedParent.pop_back();
 
     return true;
   }
 
+  bool TraverseCompoundStmt(CompoundStmt *Node) {
+    // 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;
+    Base::TraverseCompoundStmt(Node);
+    --CurrentNestingLevel;
+
+    return true;
+  }
+
   bool TraverseDecl(Decl *Node) {
     TrackedParent.push_back(false);
     Base::TraverseDecl(Node);
     TrackedParent.pop_back();
     return true;
   }
 
   struct FunctionInfo {

clang-tools-extra/trunk/test/clang-tidy/readability-function-size.cpp

 // CHECK-MESSAGES: :[[@LINE-2]]:6: note: 3 statements (threshold 0)
 //
 // CHECK-MESSAGES: :[[@LINE-4]]:30: warning: function 'barx' exceeds recommended size/complexity
 // CHECK-MESSAGES: :[[@LINE-5]]:30: note: 2 statements (threshold 0)
 
 #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-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;
     { // 3
 // CHECK-MESSAGES: :[[@LINE-1]]:5: note: nesting level 3 starts here (threshold 2)
       int c;
       { // 4
         int d;
       }
     }
   }
   { // 2
     int e;
   }
   { // 2
     { // 3
 // CHECK-MESSAGES: :[[@LINE-1]]:5: note: nesting level 3 starts here (threshold 2)
       int j;
     }
   }
   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: 22 lines including whitespace and comments (threshold 0)
+  // CHECK-MESSAGES: :[[@LINE-3]]:6: note: 18 statements (threshold 0)
+  // CHECK-MESSAGES: :[[@LINE-4]]:6: note: 6 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;
+     if (true) { // 3
+       // CHECK-MESSAGES: :[[@LINE-1]]:16: note: nesting level 3 starts here (threshold 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;
+        }
+      }
+    }
+  }
 }