[clang-tidy] readability-function-size: add VariableThreshold param.

Summary:
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.

Reviewers: hokein, xazax.hun, JonasToth, aaron.ballman, alexfh

Reviewed By: aaron.ballman

Subscribers: kimgr, Eugene.Zelenko, rnkovacs, cfe-commits

Tags: #clang-tools-extra

Differential Revision: https://reviews.llvm.org/D44602

llvm-svn: 329902
This commit is contained in:
Roman Lebedev 2018-04-12 12:06:42 +00:00
parent 53271ba1d2
commit 4d37af003f
6 changed files with 249 additions and 8 deletions

View File

@ -22,6 +22,21 @@ class FunctionASTVisitor : public RecursiveASTVisitor<FunctionASTVisitor> {
using Base = RecursiveASTVisitor<FunctionASTVisitor>;
public:
bool VisitVarDecl(VarDecl *VD) {
// Do not count function params.
// Do not count decomposition declarations (C++17's structured bindings).
if (StructNesting == 0 &&
!(isa<ParmVarDecl>(VD) || isa<DecompositionDecl>(VD)))
++Info.Variables;
return true;
}
bool VisitBindingDecl(BindingDecl *BD) {
// Do count each of the bindings (in the decomposition declaration).
if (StructNesting == 0)
++Info.Variables;
return true;
}
bool TraverseStmt(Stmt *Node) {
if (!Node)
return Base::TraverseStmt(Node);
@ -74,15 +89,38 @@ public:
return true;
}
bool TraverseLambdaExpr(LambdaExpr *Node) {
++StructNesting;
Base::TraverseLambdaExpr(Node);
--StructNesting;
return true;
}
bool TraverseCXXRecordDecl(CXXRecordDecl *Node) {
++StructNesting;
Base::TraverseCXXRecordDecl(Node);
--StructNesting;
return true;
}
bool TraverseStmtExpr(StmtExpr *SE) {
++StructNesting;
Base::TraverseStmtExpr(SE);
--StructNesting;
return true;
}
struct FunctionInfo {
unsigned Lines = 0;
unsigned Statements = 0;
unsigned Branches = 0;
unsigned NestingThreshold = 0;
unsigned Variables = 0;
std::vector<SourceLocation> NestingThresholders;
};
FunctionInfo Info;
std::vector<bool> TrackedParent;
unsigned StructNesting = 0;
unsigned CurrentNestingLevel = 0;
};
@ -94,7 +132,8 @@ FunctionSizeCheck::FunctionSizeCheck(StringRef Name, ClangTidyContext *Context)
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);
@ -102,6 +141,7 @@ void FunctionSizeCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
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) {
@ -133,7 +173,7 @@ void FunctionSizeCheck::check(const MatchFinder::MatchResult &Result) {
if (FI.Lines > LineThreshold || FI.Statements > StatementThreshold ||
FI.Branches > BranchThreshold ||
ActualNumberParameters > ParameterThreshold ||
!FI.NestingThresholders.empty()) {
!FI.NestingThresholders.empty() || FI.Variables > VariableThreshold) {
diag(Func->getLocation(),
"function %0 exceeds recommended size/complexity thresholds")
<< Func;
@ -168,6 +208,12 @@ void FunctionSizeCheck::check(const MatchFinder::MatchResult &Result) {
DiagnosticIDs::Note)
<< NestingThreshold + 1 << NestingThreshold;
}
if (FI.Variables > VariableThreshold) {
diag(Func->getLocation(), "%0 variables (threshold %1)",
DiagnosticIDs::Note)
<< FI.Variables << VariableThreshold;
}
}
} // namespace readability

View File

@ -33,6 +33,8 @@ namespace readability {
/// 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 @@ private:
const unsigned BranchThreshold;
const unsigned ParameterThreshold;
const unsigned NestingThreshold;
const unsigned VariableThreshold;
};
} // namespace readability

View File

@ -158,6 +158,11 @@ Improvements to clang-tidy
<clang-tidy/checks/cppcoreguidelines-avoid-goto>`
added.
- Added `VariableThreshold` option to :doc:`readability-function-size
<clang-tidy/checks/readability-function-size>` check
Flags functions that have more than a specified number of variables declared in the body.
- Adding the missing bitwise assignment operations to
:doc:`hicpp-signed-bitwise <clang-tidy/checks/hicpp-signed-bitwise>`.

View File

@ -36,3 +36,10 @@ Options
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).
Please note that function parameters and variables declared in lambdas,
GNU Statement Expressions, and nested class inline functions are not counted.

View File

@ -0,0 +1,21 @@
// 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++17
void structured_bindings() {
int a[2] = {1, 2};
auto [x, y] = a;
}
// CHECK-MESSAGES: :[[@LINE-4]]:6: warning: function 'structured_bindings' 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: 3 variables (threshold 1)
#define SWAP(x, y) ({auto& [x0, x1] = x; __typeof__(x) t = {x0, x1}; auto& [y0, y1] = y; auto& [t0, t1] = t; x0 = y0; x1 = y1; y0 = t0; y1 = t1; })
void variables_13() {
int a[2] = {1, 2};
int b[2] = {3, 4};
SWAP(a, b);
}
// CHECK-MESSAGES: :[[@LINE-5]]:6: warning: function 'variables_13' 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: 11 statements (threshold 0)
// CHECK-MESSAGES: :[[@LINE-8]]:6: note: 2 variables (threshold 1)

View File

@ -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 bar2() { class A { void barx() {;;} }; }
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 @@ void baz0() { // 1
}
}
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 @@ void nesting_if() { // 1
} 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,161 @@ void bad_if_nesting() { // 1
}
}
}
// 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, int j) {
;
}
// 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[2];
}
// CHECK-MESSAGES: :[[@LINE-3]]:6: warning: function 'variables_3' 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_4() {
int i;
int j;
}
// CHECK-MESSAGES: :[[@LINE-4]]:6: warning: function 'variables_4' 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_5() {
int i, j;
}
// CHECK-MESSAGES: :[[@LINE-3]]:6: warning: function 'variables_5' 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_6() {
for (int i;;)
for (int j;;)
;
}
// CHECK-MESSAGES: :[[@LINE-5]]:6: warning: function 'variables_6' 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)
void variables_7() {
if (int a = 1)
if (int b = 2)
;
}
// CHECK-MESSAGES: :[[@LINE-5]]:6: warning: function 'variables_7' 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: 7 statements (threshold 0)
// CHECK-MESSAGES: :[[@LINE-8]]:6: note: 2 branches (threshold 0)
// CHECK-MESSAGES: :[[@LINE-9]]:6: note: 2 variables (threshold 1)
void variables_8() {
int a[2];
for (auto i : a)
for (auto j : a)
;
}
// CHECK-MESSAGES: :[[@LINE-6]]:6: warning: function 'variables_8' exceeds recommended size/complexity thresholds [readability-function-size]
// CHECK-MESSAGES: :[[@LINE-7]]:6: note: 5 lines including whitespace and comments (threshold 0)
// CHECK-MESSAGES: :[[@LINE-8]]:6: note: 8 statements (threshold 0)
// CHECK-MESSAGES: :[[@LINE-9]]:6: note: 2 branches (threshold 0)
// CHECK-MESSAGES: :[[@LINE-10]]:6: note: 3 variables (threshold 1)
void variables_9() {
int a, b;
struct A {
A(int c, int d) {
int e, f;
}
};
}
// CHECK-MESSAGES: :[[@LINE-8]]:6: warning: function 'variables_9' exceeds recommended size/complexity thresholds [readability-function-size]
// CHECK-MESSAGES: :[[@LINE-9]]:6: note: 7 lines including whitespace and comments (threshold 0)
// CHECK-MESSAGES: :[[@LINE-10]]:6: note: 3 statements (threshold 0)
// CHECK-MESSAGES: :[[@LINE-11]]:6: note: 2 variables (threshold 1)
// CHECK-MESSAGES: :[[@LINE-9]]:5: warning: function 'A' exceeds recommended size/complexity thresholds [readability-function-size]
// CHECK-MESSAGES: :[[@LINE-10]]:5: note: 2 lines including whitespace and comments (threshold 0)
// CHECK-MESSAGES: :[[@LINE-11]]:5: note: 1 statements (threshold 0)
// CHECK-MESSAGES: :[[@LINE-12]]:5: note: 2 variables (threshold 1)
void variables_10() {
int a, b;
struct A {
int c;
int d;
};
}
// CHECK-MESSAGES: :[[@LINE-7]]:6: warning: function 'variables_10' exceeds recommended size/complexity thresholds [readability-function-size]
// CHECK-MESSAGES: :[[@LINE-8]]:6: note: 6 lines including whitespace and comments (threshold 0)
// CHECK-MESSAGES: :[[@LINE-9]]:6: note: 2 statements (threshold 0)
// CHECK-MESSAGES: :[[@LINE-10]]:6: note: 2 variables (threshold 1)
void variables_11() {
struct S {
void bar() {
int a, b;
}
};
}
// CHECK-MESSAGES: :[[@LINE-7]]:6: warning: function 'variables_11' exceeds recommended size/complexity thresholds [readability-function-size]
// CHECK-MESSAGES: :[[@LINE-8]]:6: note: 6 lines including whitespace and comments (threshold 0)
// CHECK-MESSAGES: :[[@LINE-7]]:10: warning: function 'bar' exceeds recommended size/complexity thresholds [readability-function-size]
// CHECK-MESSAGES: :[[@LINE-8]]:10: note: 2 lines including whitespace and comments (threshold 0)
// CHECK-MESSAGES: :[[@LINE-9]]:10: note: 2 variables (threshold 1)
void variables_12() {
int v;
auto test = [](int a, int b) -> void {};
test({}, {});
}
// CHECK-MESSAGES: :[[@LINE-5]]:6: warning: function 'variables_12' 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: 3 statements (threshold 0)
// CHECK-MESSAGES: :[[@LINE-8]]:6: note: 2 variables (threshold 1)
void variables_13() {
int v;
auto test = []() -> void {
int a;
int b;
};
test();
}
// CHECK-MESSAGES: :[[@LINE-8]]:6: warning: function 'variables_13' exceeds recommended size/complexity thresholds [readability-function-size]
// CHECK-MESSAGES: :[[@LINE-9]]:6: note: 7 lines including whitespace and comments (threshold 0)
// CHECK-MESSAGES: :[[@LINE-10]]:6: note: 5 statements (threshold 0)
// CHECK-MESSAGES: :[[@LINE-11]]:6: note: 2 variables (threshold 1)
void variables_14() {
(void)({int a = 12; a; });
(void)({int a = 12; a; });
}
// CHECK-MESSAGES: :[[@LINE-4]]:6: warning: function 'variables_14' 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: 6 statements (threshold 0)
#define SWAP(x, y) ({__typeof__(x) temp = x; x = y; y = temp; })
void variables_15() {
int a = 10, b = 12;
SWAP(a, b);
}
// CHECK-MESSAGES: :[[@LINE-4]]:6: warning: function 'variables_15' 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: 5 statements (threshold 0)
// CHECK-MESSAGES: :[[@LINE-7]]:6: note: 2 variables (threshold 1)
#define vardecl(type, name) type name;
void variables_16() {
vardecl(int, a);
vardecl(int, b);
}
// CHECK-MESSAGES: :[[@LINE-4]]:6: warning: function 'variables_16' 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: 4 statements (threshold 0)
// CHECK-MESSAGES: :[[@LINE-7]]:6: note: 2 variables (threshold 1)