[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

git-svn-id: https://llvm.org/svn/llvm-project/clang-tools-extra/trunk@329902 91177308-0d34-0410-b5e6-96231b3b80d8

2 files changed, 51 insertions, 2 deletions

diff --git a/clang-tidy/readability/FunctionSizeCheck.cpp b/clang-tidy/readability/FunctionSizeCheck.cpp
index e6410ce2..8ed6b907 100644
--- a/clang-tidy/readability/FunctionSizeCheck.cpp
+++ b/clang-tidy/readability/FunctionSizeCheck.cpp
@@ -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
diff --git a/clang-tidy/readability/FunctionSizeCheck.h b/clang-tidy/readability/FunctionSizeCheck.h
index 3986b95a..7defccdb 100644
--- a/clang-tidy/readability/FunctionSizeCheck.h
+++ b/clang-tidy/readability/FunctionSizeCheck.h
@@ -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