lebedev.ri created this revision.
lebedev.ri added reviewers: alexfh, hokein, xazax.hun, JonasToth, aaron.ballman.
lebedev.ri added a project: clang-tools-extra.
Herald added a subscriber: rnkovacs.
Pretty straight-forward, just count all the variable declarations in the
function's body, and if more than the configured threshold - do complain.
Note that this continues perverse practice of disabling the new option by
default.
I'm not certain where is the balance point between not being too noisy, and
actually enforcing the good practice.
If we really want to not disable this by default, but also to not cause too
many new warnings, we could default to 50 or so.
But that is a lot of variables too...
I was able to find one coding style referencing variable count:
- https://www.kernel.org/doc/html/v4.15/process/coding-style.html#functions
> Another measure of the function is the number of local variables. They
shouldn’t exceed 5-10, or you’re doing something wrong.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D44602
Files:
clang-tidy/readability/FunctionSizeCheck.cpp
clang-tidy/readability/FunctionSizeCheck.h
docs/ReleaseNotes.rst
docs/clang-tidy/checks/readability-function-size.rst
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
@@ -1,4 +1,4 @@
-// RUN: %check_clang_tidy %s readability-function-size %t -- -config='{CheckOptions: [{key: readability-function-size.LineThreshold, value: 0}, {key: readability-function-size.StatementThreshold, value: 0}, {key: readability-function-size.BranchThreshold, value: 0}, {key: readability-function-size.ParameterThreshold, value: 5}, {key: readability-function-size.NestingThreshold, value: 2}]}' -- -std=c++11
+// RUN: %check_clang_tidy %s readability-function-size %t -- -config='{CheckOptions: [{key: readability-function-size.LineThreshold, value: 0}, {key: readability-function-size.StatementThreshold, value: 0}, {key: readability-function-size.BranchThreshold, value: 0}, {key: readability-function-size.ParameterThreshold, value: 5}, {key: readability-function-size.NestingThreshold, value: 2}, {key: readability-function-size.VariableThreshold, value: 1}]}' -- -std=c++11
// Bad formatting is intentional, don't run clang-format over the whole file!
@@ -64,7 +64,7 @@
void baz0() { // 1
// 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-2]]:6: note: 28 lines including whitespace and comments (threshold 0)
// CHECK-MESSAGES: :[[@LINE-3]]:6: note: 9 statements (threshold 0)
int a;
{ // 2
@@ -87,14 +87,15 @@
}
}
macro()
-// CHECK-MESSAGES: :[[@LINE-1]]:3: note: nesting level 3 starts here (threshold 2)
-// CHECK-MESSAGES: :[[@LINE-28]]:25: note: expanded from macro 'macro'
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: note: nesting level 3 starts here (threshold 2)
+ // CHECK-MESSAGES: :[[@LINE-28]]:25: note: expanded from macro 'macro'
+ // CHECK-MESSAGES: :[[@LINE-27]]:6: note: 9 variables (threshold 1)
}
// 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-2]]:6: note: 23 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
@@ -114,12 +115,13 @@
} else if (true) { // 2
int j;
}
+ // CHECK-MESSAGES: :[[@LINE-22]]:6: note: 6 variables (threshold 1)
}
// 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-2]]:6: note: 23 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
@@ -139,4 +141,49 @@
}
}
}
+ // CHECK-MESSAGES: :[[@LINE-22]]:6: note: 4 variables (threshold 1)
}
+
+void variables_0() {
+ int i;
+}
+// CHECK-MESSAGES: :[[@LINE-3]]:6: warning: function 'variables_0' exceeds recommended size/complexity thresholds [readability-function-size]
+// CHECK-MESSAGES: :[[@LINE-4]]:6: note: 2 lines including whitespace and comments (threshold 0)
+// CHECK-MESSAGES: :[[@LINE-5]]:6: note: 1 statements (threshold 0)
+void variables_1(int i) {
+ int j;
+}
+// CHECK-MESSAGES: :[[@LINE-3]]:6: warning: function 'variables_1' exceeds recommended size/complexity thresholds [readability-function-size]
+// CHECK-MESSAGES: :[[@LINE-4]]:6: note: 2 lines including whitespace and comments (threshold 0)
+// CHECK-MESSAGES: :[[@LINE-5]]:6: note: 1 statements (threshold 0)
+void variables_2() {
+ int i[2];
+}
+// CHECK-MESSAGES: :[[@LINE-3]]:6: warning: function 'variables_2' exceeds recommended size/complexity thresholds [readability-function-size]
+// CHECK-MESSAGES: :[[@LINE-4]]:6: note: 2 lines including whitespace and comments (threshold 0)
+// CHECK-MESSAGES: :[[@LINE-5]]:6: note: 1 statements (threshold 0)
+void variables_3() {
+ int i;
+ int j;
+}
+// CHECK-MESSAGES: :[[@LINE-4]]:6: warning: function 'variables_3' exceeds recommended size/complexity thresholds [readability-function-size]
+// CHECK-MESSAGES: :[[@LINE-5]]:6: note: 3 lines including whitespace and comments (threshold 0)
+// CHECK-MESSAGES: :[[@LINE-6]]:6: note: 2 statements (threshold 0)
+// CHECK-MESSAGES: :[[@LINE-7]]:6: note: 2 variables (threshold 1)
+void variables_4() {
+ int i, j;
+}
+// CHECK-MESSAGES: :[[@LINE-3]]:6: warning: function 'variables_4' exceeds recommended size/complexity thresholds [readability-function-size]
+// CHECK-MESSAGES: :[[@LINE-4]]:6: note: 2 lines including whitespace and comments (threshold 0)
+// CHECK-MESSAGES: :[[@LINE-5]]:6: note: 1 statements (threshold 0)
+// CHECK-MESSAGES: :[[@LINE-6]]:6: note: 2 variables (threshold 1)
+void variables_5() {
+ for (int i;;)
+ for (int j;;)
+ ;
+}
+// CHECK-MESSAGES: :[[@LINE-5]]:6: warning: function 'variables_5' exceeds recommended size/complexity thresholds [readability-function-size]
+// CHECK-MESSAGES: :[[@LINE-6]]:6: note: 4 lines including whitespace and comments (threshold 0)
+// CHECK-MESSAGES: :[[@LINE-7]]:6: note: 5 statements (threshold 0)
+// CHECK-MESSAGES: :[[@LINE-8]]:6: note: 2 branches (threshold 0)
+// CHECK-MESSAGES: :[[@LINE-9]]:6: note: 2 variables (threshold 1)
Index: docs/clang-tidy/checks/readability-function-size.rst
===================================================================
--- docs/clang-tidy/checks/readability-function-size.rst
+++ docs/clang-tidy/checks/readability-function-size.rst
@@ -36,3 +36,8 @@
Flag compound statements which create next nesting level after
`NestingThreshold`. This may differ significantly from the expected value
for macro-heavy code. The default is `-1` (ignore the nesting level).
+
+.. option:: VariableThreshold
+
+ Flag functions exceeding this number of variables declared in the body.
+ The default is `-1` (ignore the number of variables).
Index: docs/ReleaseNotes.rst
===================================================================
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -119,6 +119,11 @@
Warns on construction of specific temporary objects in the Zircon kernel.
+- Added `VariableThreshold` to `readability-function-size
+ <http://clang.llvm.org/extra/clang-tidy/checks/readability-function-size.html>`_ check
+
+ Flags functions exceeding this number of variables declared in the body.
+
- New alias `hicpp-avoid-goto
<http://clang.llvm.org/extra/clang-tidy/checks/hicpp-avoid-goto.html>`_ to
`cppcoreguidelines-avoid-goto <http://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines-avoid-goto.html>`_
Index: clang-tidy/readability/FunctionSizeCheck.h
===================================================================
--- clang-tidy/readability/FunctionSizeCheck.h
+++ clang-tidy/readability/FunctionSizeCheck.h
@@ -33,6 +33,8 @@
/// level after `NestingThreshold`. This may differ significantly from the
/// expected value for macro-heavy code. The default is `-1` (ignore the
/// nesting level).
+/// * `VariableThreshold` - flag functions having a high number of variable
+/// declarations. The default is `-1` (ignore the number of variables).
class FunctionSizeCheck : public ClangTidyCheck {
public:
FunctionSizeCheck(StringRef Name, ClangTidyContext *Context);
@@ -47,6 +49,7 @@
const unsigned BranchThreshold;
const unsigned ParameterThreshold;
const unsigned NestingThreshold;
+ const unsigned VariableThreshold;
};
} // namespace readability
Index: clang-tidy/readability/FunctionSizeCheck.cpp
===================================================================
--- clang-tidy/readability/FunctionSizeCheck.cpp
+++ clang-tidy/readability/FunctionSizeCheck.cpp
@@ -22,6 +22,11 @@
using Base = RecursiveASTVisitor<FunctionASTVisitor>;
public:
+ bool VisitVarDecl(VarDecl *) {
+ Info.Variables++;
+ return true;
+ }
+
bool TraverseStmt(Stmt *Node) {
if (!Node)
return Base::TraverseStmt(Node);
@@ -79,6 +84,7 @@
unsigned Statements = 0;
unsigned Branches = 0;
unsigned NestingThreshold = 0;
+ unsigned Variables = 0;
std::vector<SourceLocation> NestingThresholders;
};
FunctionInfo Info;
@@ -94,14 +100,16 @@
StatementThreshold(Options.get("StatementThreshold", 800U)),
BranchThreshold(Options.get("BranchThreshold", -1U)),
ParameterThreshold(Options.get("ParameterThreshold", -1U)),
- NestingThreshold(Options.get("NestingThreshold", -1U)) {}
+ NestingThreshold(Options.get("NestingThreshold", -1U)),
+ VariableThreshold(Options.get("VariableThreshold", -1U)) {}
void FunctionSizeCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
Options.store(Opts, "LineThreshold", LineThreshold);
Options.store(Opts, "StatementThreshold", StatementThreshold);
Options.store(Opts, "BranchThreshold", BranchThreshold);
Options.store(Opts, "ParameterThreshold", ParameterThreshold);
Options.store(Opts, "NestingThreshold", NestingThreshold);
+ Options.store(Opts, "VariableThreshold", VariableThreshold);
}
void FunctionSizeCheck::registerMatchers(MatchFinder *Finder) {
@@ -129,11 +137,12 @@
}
unsigned ActualNumberParameters = Func->getNumParams();
+ unsigned BodyVariables = FI.Variables - ActualNumberParameters;
if (FI.Lines > LineThreshold || FI.Statements > StatementThreshold ||
FI.Branches > BranchThreshold ||
ActualNumberParameters > ParameterThreshold ||
- !FI.NestingThresholders.empty()) {
+ !FI.NestingThresholders.empty() || BodyVariables > VariableThreshold) {
diag(Func->getLocation(),
"function %0 exceeds recommended size/complexity thresholds")
<< Func;
@@ -168,6 +177,12 @@
DiagnosticIDs::Note)
<< NestingThreshold + 1 << NestingThreshold;
}
+
+ if (BodyVariables > VariableThreshold) {
+ diag(Func->getLocation(), "%0 variables (threshold %1)",
+ DiagnosticIDs::Note)
+ << BodyVariables << VariableThreshold;
+ }
}
} // namespace readability
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
