[PATCH] D34202: [clang-tidy] readability-function-size: fix nesting level calculation
This revision was automatically updated to reflect the committed changes. Closed by commit rL305554: [clang-tidy] readability-function-size: fix nesting level calculation (authored by lebedevri). Changed prior to commit: https://reviews.llvm.org/D34202?vs=102548=102813#toc Repository: rL LLVM https://reviews.llvm.org/D34202 Files: clang-tools-extra/trunk/clang-tidy/readability/FunctionSizeCheck.cpp clang-tools-extra/trunk/test/clang-tidy/readability-function-size.cpp Index: clang-tools-extra/trunk/clang-tidy/readability/FunctionSizeCheck.cpp === --- clang-tools-extra/trunk/clang-tidy/readability/FunctionSizeCheck.cpp +++ clang-tools-extra/trunk/clang-tidy/readability/FunctionSizeCheck.cpp @@ -36,15 +36,8 @@ 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,13 +47,25 @@ 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); Index: clang-tools-extra/trunk/test/clang-tidy/readability-function-size.cpp === --- clang-tools-extra/trunk/test/clang-tidy/readability-function-size.cpp +++ clang-tools-extra/trunk/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,55 @@ } 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; +} + } +} + } }
[PATCH] D34202: [clang-tidy] readability-function-size: fix nesting level calculation
malcolm.parsons added a comment. LGTM! Comment at: clang-tidy/readability/FunctionSizeCheck.cpp:58 +// is already nested NestingThreshold levels deep, record the start location +// of this new compound statement +if (CurrentNestingLevel == Info.NestingThreshold) Missing full stop. Repository: rL LLVM https://reviews.llvm.org/D34202 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34202: [clang-tidy] readability-function-size: fix nesting level calculation
lebedev.ri added a comment. In https://reviews.llvm.org/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... Repository: rL LLVM https://reviews.llvm.org/D34202 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34202: [clang-tidy] readability-function-size: fix nesting level calculation
malcolm.parsons added a comment. My coding standards require the `{}`, so while I may not agree with your definition of nesting, it doesn't matter. Repository: rL LLVM https://reviews.llvm.org/D34202 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34202: [clang-tidy] readability-function-size: fix nesting level calculation
lebedev.ri added a comment. In https://reviews.llvm.org/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 ... Repository: rL LLVM https://reviews.llvm.org/D34202 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34202: [clang-tidy] readability-function-size: fix nesting level calculation
malcolm.parsons added a comment. What do you expect for this? if (true) if (true) if (true) { int j; } Repository: rL LLVM https://reviews.llvm.org/D34202 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34202: [clang-tidy] readability-function-size: fix nesting level calculation
lebedev.ri updated this revision to Diff 102548. lebedev.ri added a comment. Simplify it even further by moving the logic into it's proper place - `TraverseCompoundStmt()` 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,55 @@ } 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; +} + } +} + } } Index: clang-tidy/readability/FunctionSizeCheck.cpp === --- clang-tidy/readability/FunctionSizeCheck.cpp +++ clang-tidy/readability/FunctionSizeCheck.cpp @@ -36,15 +36,8 @@ 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,13 +47,25 @@ 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); ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34202: [clang-tidy] readability-function-size: fix nesting level calculation
lebedev.ri updated this revision to Diff 102542. lebedev.ri marked an inline comment as done. lebedev.ri added a comment. Fix spelling. 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,55 @@ } 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; +} + } +} + } } Index: clang-tidy/readability/FunctionSizeCheck.cpp === --- clang-tidy/readability/FunctionSizeCheck.cpp +++ clang-tidy/readability/FunctionSizeCheck.cpp @@ -25,8 +25,20 @@ if (!Node) return Base::TraverseStmt(Node); -if (TrackedParent.back() && !isa(Node)) - ++Info.Statements; +if (isa(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()); + + // We have just entered CompoundStmt, increase current nesting level. + ++CurrentNestingLevel; +} else { + assert(!isa(Node)); + if (TrackedParent.back()) +++Info.Statements; +} switch (Node->getStmtClass()) { case Stmt::IfStmtClass: @@ -36,15 +48,8 @@ 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,8 +59,10 @@ Base::TraverseStmt(Node); -if (TrackedParent.back()) +// Did we just process CompoundStmt? if yes, decrease current nesting level. +if (isa(Node)) --CurrentNestingLevel; + TrackedParent.pop_back(); return true; ___
[PATCH] D34202: [clang-tidy] readability-function-size: fix nesting level calculation
lebedev.ri added inline comments. Comment at: clang-tidy/readability/FunctionSizeCheck.cpp:38 +} else { + assert(!isa(Node)); + if (TrackedParent.back()) malcolm.parsons wrote: > I don't think this assert adds anything. Yes, however the original `if` did have that check. I **think**, having an assert here may help to not break this in the future. Repository: rL LLVM https://reviews.llvm.org/D34202 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34202: [clang-tidy] readability-function-size: fix nesting level calculation
malcolm.parsons added inline comments. Comment at: clang-tidy/readability/FunctionSizeCheck.cpp:38 +} else { + assert(!isa(Node)); + if (TrackedParent.back()) I don't think this assert adds anything. Comment at: clang-tidy/readability/FunctionSizeCheck.cpp:62 -if (TrackedParent.back()) +// did we just process CompoundStmt? if yes, decrease current nesting level. +if (isa(Node)) Comments are sentences, so start with a capital letter. Repository: rL LLVM https://reviews.llvm.org/D34202 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34202: [clang-tidy] readability-function-size: fix nesting level calculation
lebedev.ri updated this revision to Diff 102533. lebedev.ri edited the summary of this revision. lebedev.ri added a comment. Ok, i really messed up the initial https://reviews.llvm.org/D32942 :) Malcolm, thank you for bothering to report :) This should be so much better. 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,55 @@ } 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; +} + } +} + } } Index: clang-tidy/readability/FunctionSizeCheck.cpp === --- clang-tidy/readability/FunctionSizeCheck.cpp +++ clang-tidy/readability/FunctionSizeCheck.cpp @@ -25,8 +25,20 @@ if (!Node) return Base::TraverseStmt(Node); -if (TrackedParent.back() && !isa(Node)) - ++Info.Statements; +if (isa(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()); + + // We have just entered CompoundStmt, increase current nesting level. + ++CurrentNestingLevel; +} else { + assert(!isa(Node)); + if (TrackedParent.back()) +++Info.Statements; +} switch (Node->getStmtClass()) { case Stmt::IfStmtClass: @@ -36,15 +48,8 @@ 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,8 +59,10 @@ Base::TraverseStmt(Node); -if (TrackedParent.back()) +// did we just process CompoundStmt? if yes, decrease current nesting level. +if (isa(Node))
[PATCH] D34202: [clang-tidy] readability-function-size: fix nesting level calculation
lebedev.ri planned changes to this revision. lebedev.ri added a comment. Hmm, this is moving in the right direction, but now it invalidated (decreases the nesting level) too eagerly Repository: rL LLVM https://reviews.llvm.org/D34202 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34202: [clang-tidy] readability-function-size: fix nesting level calculation
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: 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: