[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:
