[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

1 files changed, 48 insertions, 2 deletions

diff --git a/clang-tools-extra/clang-tidy/readability/FunctionSizeCheck.cpp b/clang-tools-extra/clang-tidy/readability/FunctionSizeCheck.cpp
index e6410ce2134..8ed6b90773a 100644
--- a/clang-tools-extra/clang-tidy/readability/FunctionSizeCheck.cpp
+++ b/clang-tools-extra/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