Author: lebedevri Date: Fri Jun 16 08:07:47 2017 New Revision: 305554 URL: http://llvm.org/viewvc/llvm-project?rev=305554&view=rev Log: [clang-tidy] readability-function-size: fix nesting level calculation
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. Reviewers: malcolm.parsons, alexfh Reviewed By: malcolm.parsons Subscribers: sbenza, xazax.hun, cfe-commits, aaron.ballman Tags: #clang-tools-extra Differential Revision: https://reviews.llvm.org/D34202 Modified: clang-tools-extra/trunk/clang-tidy/readability/FunctionSizeCheck.cpp clang-tools-extra/trunk/test/clang-tidy/readability-function-size.cpp Modified: clang-tools-extra/trunk/clang-tidy/readability/FunctionSizeCheck.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/readability/FunctionSizeCheck.cpp?rev=305554&r1=305553&r2=305554&view=diff ============================================================================== --- clang-tools-extra/trunk/clang-tidy/readability/FunctionSizeCheck.cpp (original) +++ clang-tools-extra/trunk/clang-tidy/readability/FunctionSizeCheck.cpp Fri Jun 16 08:07:47 2017 @@ -36,15 +36,8 @@ public: 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: @@ -54,12 +47,24 @@ public: 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); Modified: clang-tools-extra/trunk/test/clang-tidy/readability-function-size.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-tidy/readability-function-size.cpp?rev=305554&r1=305553&r2=305554&view=diff ============================================================================== --- clang-tools-extra/trunk/test/clang-tidy/readability-function-size.cpp (original) +++ clang-tools-extra/trunk/test/clang-tidy/readability-function-size.cpp Fri Jun 16 08:07:47 2017 @@ -63,8 +63,9 @@ void bar2() { class A { void barx() {;;} #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,55 @@ void baz0() { // 1 } 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; + } + } + } + } } _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits