[clang-tidy] add readability-compound-statement-size check.

[lebedev.ri]

Summary

This check is quite similar to the readability-function-size
check. But it is more generic. It works for each compound
statement, excluding the ones directly related to the function,
i.e. it does not duplicate the readability-function-size output.

The rationale behind the check is the same as behind
readability-function-size check, it may make sense to be able
to control sizes of the compound statements, not just the
size of the functions.

Eventually, i would like these two checkers to be one, and
handle more cases and be more configurable, e.g. to be able to
apply such a check to the 'True' block of 'If' and so on.

But since this is my first attempt at any llvm/clang patch,
this is what it is. Please do review carefully.

[lebedev.ri]

Do note that clang-tidy/readability/CompoundStatementSizeCheck.cpp is *very* similar to the clang-tidy/readability/FunctionSizeCheck.cpp
I'm not sure if it makes sense to have such duplication. But i'm also not sure yet how to deduplicate them.
But in the long run, i would like these two checks to be one, and much more configurable.

[JonasToth]

Hi and welcome to the clang/llvm community :)

It seems that you took readability-size as starting point and modified it. Thats a good way to learn!

My opinion is, that this should land in readability-function-size. This is somewhat recursive to function size. Whenever there is a compundstmt, the same (or maybe other constraints) exist for that compound statement.
Thats, in my opinion, the connecting factor to function-size.
It should be configurable. Maybe something like "global" constaint on the function (e.g. max 800 lines in total) and "local" constraints for subcomponents. These two layers of configurability kinda already exist. Your check would implement the recursive and local part, and function size the global.

What are your thoughts on that? But generally i like that approach to limit subparts of the functions as well.
Recommending to refactor such code to use functions would be a good diagnostic in my opinion.

When creating or modifying checks it would be nice to include a note in the ReleaseNotes as well (in doc/ i believe), but i think we should discuss the placement of this functionality first.

[JonasToth]

Another thought from me:

Maybe it would be sensible to create a check like complexity-limits where different forms of complicated constructs are examined. This could include extreme inheritance, excessive amount of members in classes (violation of single responsibility), very long conditions and these kind of things.
Function size would be another part of complexity and the traversing of all statements in the code would be done once. This would be good for performance as well, one problem would be the complexity of the check though ;D

Maybe we could even move this to the mail list, there would be good input as well.

[Eugene.Zelenko]

Please mention this check in docs/ReleaseNotes.rst (in alphabetical order).

Will be good idea to run Clang-tidy modernize and readability checks over new code.

clang-tidy/readability/CompoundStatementSizeCheck.cpp

42

Please use LLVM_FALLTHROUGH instead (defined in llvm/Support/Compiler.h)

65

Please use default member initializers and = default;

[lebedev.ri]

Addressing review notes.

[lebedev.ri]

No changes compared to v2, just correctly rebased the master branch now.

[lebedev.ri]

In D31252#708209, @Eugene.Zelenko wrote:

Please mention this check in docs/ReleaseNotes.rst (in alphabetical order).

Done

Will be good idea to run Clang-tidy modernize and readability checks over new code.

I did run them (-checks=*) over CompoundStatementSizeCheck.cpp, there is no modernize notes, and no meaningful readability notes.

[alexfh]

Hi Roman,

Welcome to the community! As others noted, adding a separate check so similar functionally and implementation-wise to the existing one is not the best way to go here. A single check for all similar complexity limits would be a better solution. However, first I'd like to understand the specific use case you have for this check. Is there a recommendation of a certain style document to impose a complexity limit per-compound statement instead of a function? Should both limits be used? What would be the specific numbers and how should they interact with regard to nesting (e.g. should the warning be issued only on the innermost compound statement violating the rule or should it be issued on all levels)?

[lebedev.ri]

Thank you all!
After some thought, i have come to conclusion that it is best to start with something less controversial, and simpler.

clang-tidy/readability/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyReadabilityModule
   AvoidConstParamsInDecls.cpp
   BracesAroundStatementsCheck.cpp
+  CompoundStatementSizeCheck.cpp
   ContainerSizeEmptyCheck.cpp
   DeleteNullPointerCheck.cpp
   DeletedDefaultCheck.cpp
   ElseAfterReturnCheck.cpp
   FunctionSizeCheck.cpp
   IdentifierNamingCheck.cpp
   ImplicitBoolCastCheck.cpp
   InconsistentDeclarationParameterNameCheck.cpp

clang-tidy/readability/CompoundStatementSizeCheck.h

+//===--- CompoundStatementSizeCheck.h - clang-tidy --------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_COMPOUNDSTATEMENTSIZE_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_COMPOUNDSTATEMENTSIZE_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace readability {
+
+/// Checks for large compound statements based on various metrics.
+///
+/// These options are supported:
+///
+///   * `LineThreshold` - flag compound statements exceeding this number of
+///     lines. The default is `-1` (ignore the number of lines).
+///   * `StatementThreshold` - flag compound statements exceeding this number of
+///     statements. This may differ significantly from the number of lines for
+///     macro-heavy code. The default is `800`.
+///   * `BranchThreshold` - flag compound statements exceeding this number of
+///     control statements. The default is `-1` (ignore the number of branches).
+class CompoundStatementSizeCheck : public ClangTidyCheck {
+public:
+  CompoundStatementSizeCheck(StringRef Name, ClangTidyContext *Context);
+
+  void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+
+private:
+  const unsigned LineThreshold;
+  const unsigned StatementThreshold;
+  const unsigned BranchThreshold;
+};
+
+} // namespace readability
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_COMPOUNDSTATEMENTSIZE_H

clang-tidy/readability/CompoundStatementSizeCheck.cpp

+//===--- CompoundStatement.cpp - clang-tidy
+//------------------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "CompoundStatementSizeCheck.h"
+#include "clang/AST/RecursiveASTVisitor.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace readability {
+
+class CompoundStatementASTVisitor
+    : public RecursiveASTVisitor<CompoundStatementASTVisitor> {
+  using Base = RecursiveASTVisitor<CompoundStatementASTVisitor>;
+
+public:
+  bool TraverseStmt(Stmt *Node) {
+    if (!Node)
+      return Base::TraverseStmt(Node);
+
+    if (TrackedParent.back() && !isa<CompoundStmt>(Node))
+      ++Info.Statements;
+
+    switch (Node->getStmtClass()) {
+    case Stmt::IfStmtClass:
+    case Stmt::WhileStmtClass:
+    case Stmt::DoStmtClass:
+    case Stmt::CXXForRangeStmtClass:
+    case Stmt::ForStmtClass:
+    case Stmt::SwitchStmtClass:
+      ++Info.Branches;
+      LLVM_FALLTHROUGH;
+    case Stmt::CompoundStmtClass:
+      TrackedParent.push_back(true);
+      break;
+    default:
+      TrackedParent.push_back(false);
+      break;
+    }
+
+    Base::TraverseStmt(Node);
+
+    TrackedParent.pop_back();
+    return true;
+  }
+
+  bool TraverseCompoundStatement(CompoundStmt *Node) {
+    TrackedParent.push_back(true);
+    Base::TraverseCompoundStmt(Node);
+    TrackedParent.pop_back();
+    return true;
+  }
+
+  struct CompoundStatementInfo {
+    unsigned Lines = 0;
+    unsigned Statements = 0;
+    unsigned Branches = 0;
+  };
+  CompoundStatementInfo Info;
+  std::vector<bool> TrackedParent;
+};
+
+CompoundStatementSizeCheck::CompoundStatementSizeCheck(
+    StringRef Name, ClangTidyContext *Context)
+    : ClangTidyCheck(Name, Context),
+      LineThreshold(Options.get("LineThreshold", -1U)),
+      StatementThreshold(Options.get("StatementThreshold", 800U)),
+      BranchThreshold(Options.get("BranchThreshold", -1U)) {}
+
+void CompoundStatementSizeCheck::storeOptions(
+    ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "LineThreshold", LineThreshold);
+  Options.store(Opts, "StatementThreshold", StatementThreshold);
+  Options.store(Opts, "BranchThreshold", BranchThreshold);
+}
+
+void CompoundStatementSizeCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(
+      compoundStmt(unless(hasParent(functionDecl()))).bind("compound"), this);
+}
+
+void CompoundStatementSizeCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *CS = Result.Nodes.getNodeAs<CompoundStmt>("compound");
+
+  CompoundStatementASTVisitor Visitor;
+  Visitor.TraverseCompoundStatement(const_cast<CompoundStmt *>(CS));
+  auto &CSI = Visitor.Info;
+
+  if (CSI.Statements == 0)
+    return;
+
+  // Count the lines including whitespace and comments. Really simple.
+  {
+    SourceManager *SM = Result.SourceManager;
+    CSI.Lines = SM->getSpellingLineNumber(CS->getLocEnd()) -
+                SM->getSpellingLineNumber(CS->getLocStart());
+  }
+
+  if (CSI.Lines > LineThreshold || CSI.Statements > StatementThreshold ||
+      CSI.Branches > BranchThreshold) {
+    diag(CS->getLocStart(),
+         "compound statement exceeds recommended size/complexity thresholds");
+  }
+
+  if (CSI.Lines > LineThreshold) {
+    diag(CS->getLocStart(),
+         "%0 lines including whitespace and comments (threshold %1)",
+         DiagnosticIDs::Note)
+        << CSI.Lines << LineThreshold;
+  }
+
+  if (CSI.Statements > StatementThreshold) {
+    diag(CS->getLocStart(), "%0 statements (threshold %1)", DiagnosticIDs::Note)
+        << CSI.Statements << StatementThreshold;
+  }
+
+  if (CSI.Branches > BranchThreshold) {
+    diag(CS->getLocStart(), "%0 branches (threshold %1)", DiagnosticIDs::Note)
+        << CSI.Branches << BranchThreshold;
+  }
+}
+
+} // namespace readability
+} // namespace tidy
+} // namespace clang

clang-tidy/readability/ReadabilityTidyModule.cpp

 //===--- ReadabilityTidyModule.cpp - clang-tidy ---------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
 #include "AvoidConstParamsInDecls.h"
 #include "BracesAroundStatementsCheck.h"
+#include "CompoundStatementSizeCheck.h"
 #include "ContainerSizeEmptyCheck.h"
 #include "DeleteNullPointerCheck.h"
 #include "DeletedDefaultCheck.h"
 #include "ElseAfterReturnCheck.h"
 #include "FunctionSizeCheck.h"
 #include "IdentifierNamingCheck.h"
 #include "ImplicitBoolCastCheck.h"
 #include "InconsistentDeclarationParameterNameCheck.h"

 
 class ReadabilityModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<AvoidConstParamsInDecls>(
         "readability-avoid-const-params-in-decls");
     CheckFactories.registerCheck<BracesAroundStatementsCheck>(
         "readability-braces-around-statements");
+    CheckFactories.registerCheck<CompoundStatementSizeCheck>(
+        "readability-compound-statement-size");
     CheckFactories.registerCheck<ContainerSizeEmptyCheck>(
         "readability-container-size-empty");
     CheckFactories.registerCheck<DeleteNullPointerCheck>(
         "readability-delete-null-pointer");
     CheckFactories.registerCheck<DeletedDefaultCheck>(
         "readability-deleted-default");
     CheckFactories.registerCheck<ElseAfterReturnCheck>(
         "readability-else-after-return");

docs/ReleaseNotes.rst

- New `cert-dcl58-cpp
 
   Finds modification of the ``std`` or ``posix`` namespace.
 
 - Improved `cppcoreguidelines-no-malloc 
   <http://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines-no-malloc.html>`_ check
 
   Allow custom memory management functions to be considered as well.
 
+- New `readability-compound-statement-size
+  <http://clang.llvm.org/extra/clang-tidy/checks/readability-compound-statement-size.html>`_ check
+
+  Similar to `readability-function-size
+  <http://clang.llvm.org/extra/clang-tidy/checks/readability-function-size.html>`_ check,
+  but works on all the compound statements, not just the functions.
+
 - New `readability-misleading-indentation
   <http://clang.llvm.org/extra/clang-tidy/checks/readability-misleading-indentation.html>`_ check
 
   Finds misleading indentation where braces should be introduced or the code should be reformatted.
 
 - Added `ParameterThreshold` to `readability-function-size`.
 
   Finds functions that have more then `ParameterThreshold` parameters and emits a warning.

docs/clang-tidy/checks/list.rst

.. toctree::
    performance-for-range-copy
    performance-implicit-cast-in-loop
    performance-inefficient-string-concatenation
    performance-type-promotion-in-math-fn
    performance-unnecessary-copy-initialization
    performance-unnecessary-value-param
    readability-avoid-const-params-in-decls
    readability-braces-around-statements
+   readability-compound-statement-size
    readability-container-size-empty
    readability-delete-null-pointer
    readability-deleted-default
    readability-else-after-return
    readability-function-size
    readability-identifier-naming
    readability-implicit-bool-cast
    readability-inconsistent-declaration-parameter-name

docs/clang-tidy/checks/readability-compound-statement-size.rst

+.. title:: clang-tidy - readability-compound-statement-size
+
+readability-compound-statement-size
+===================================
+
+Checks for compound statements based on various metrics.
+
+Similar to the `readability-function-size` check. It works for each compound
+statement, excluding the ones directly related to the function,
+i.e. it does not duplicate the readability-function-size output.
+
+Options
+-------
+
+.. option:: LineThreshold
+
+   Flag compound statements exceeding this number of lines.
+   The default is `-1` (ignore the number of lines).
+
+.. option:: StatementThreshold
+
+   Flag compound statements exceeding this number of statements. This may
+   differ significantly from the number of lines for macro-heavy code.
+   The default is `800`.
+
+.. option:: BranchThreshold
+
+   Flag compound statements exceeding this number of control statements.
+   The default is `-1` (ignore the number of branches).

test/clang-tidy/readability-compound-statement-size.cpp

+// RUN: %check_clang_tidy %s readability-compound-statement-size %t -- -config='{CheckOptions: [{key: readability-compound-statement-size.LineThreshold, value: 0}, {key: readability-compound-statement-size.StatementThreshold, value: 0}, {key: readability-compound-statement-size.BranchThreshold, value: 0}]}' -- -std=c++11
+
+// Bad formatting is intentional, don't run clang-format over the whole file!
+
+// the function's base compound statement is NOT being checked.
+
+void foo1() {
+}
+
+void foo2() {;}
+
+void bar2() {{;}}
+// CHECK-MESSAGES: :[[@LINE-1]]:14: warning: compound statement exceeds recommended size/complexity thresholds [readability-compound-statement-size]
+// CHECK-MESSAGES: :[[@LINE-2]]:14: note: 1 statements (threshold 0)
+
+void foo3() {
+;
+
+}
+
+void bar3() {
+  {
+    ;
+
+  }
+}
+// CHECK-MESSAGES: :[[@LINE-5]]:3: warning: compound statement exceeds recommended size/complexity thresholds [readability-compound-statement-size]
+// CHECK-MESSAGES: :[[@LINE-6]]:3: note: 3 lines including whitespace and comments (threshold 0)
+// CHECK-MESSAGES: :[[@LINE-7]]:3: note: 1 statements (threshold 0)
+
+void foo4(int i) { if (i) {} else; {}
+}
+
+void bar4(int i) { { if (i) {} else; {}
+}
+}
+// CHECK-MESSAGES: :[[@LINE-3]]:20: warning: compound statement exceeds recommended size/complexity thresholds [readability-compound-statement-size]
+// CHECK-MESSAGES: :[[@LINE-4]]:20: note: 1 lines including whitespace and comments (threshold 0)
+// CHECK-MESSAGES: :[[@LINE-5]]:20: note: 3 statements (threshold 0)
+// CHECK-MESSAGES: :[[@LINE-6]]:20: note: 1 branches (threshold 0)
+
+void foo5(int i) {for(;i;)while(i)
+do;while(i);
+}
+
+void bar5(int i) {{for(;i;)while(i)
+do;while(i);
+}
+}
+// CHECK-MESSAGES: :[[@LINE-4]]:19: warning: compound statement exceeds recommended size/complexity thresholds [readability-compound-statement-size]
+// CHECK-MESSAGES: :[[@LINE-5]]:19: note: 2 lines including whitespace and comments (threshold 0)
+// CHECK-MESSAGES: :[[@LINE-6]]:19: note: 7 statements (threshold 0)
+// CHECK-MESSAGES: :[[@LINE-7]]:19: note: 3 branches (threshold 0)
+
+template <typename T> T foo6(T i) {return i;
+}
+int x = foo6(0);
+
+template <typename T> T bar6(T i) {{return i;
+}
+}
+int y = bar6(0);
+// CHECK-MESSAGES: :[[@LINE-4]]:36: warning: compound statement exceeds recommended size/complexity thresholds [readability-compound-statement-size]
+// CHECK-MESSAGES: :[[@LINE-5]]:36: note: 1 lines including whitespace and comments (threshold 0)
+// CHECK-MESSAGES: :[[@LINE-6]]:36: note: 1 statements (threshold 0)
+
+void foo7(int p1, int p2, int p3, int p4, int p5, int p6) {;}
+
+void bar8() { [](){;;;;;;;;;;;  if(1){}
+
+
+}();
+
+
+}
+// CHECK-MESSAGES: :[[@LINE-7]]:19: warning: compound statement exceeds recommended size/complexity thresholds [readability-compound-statement-size]
+// CHECK-MESSAGES: :[[@LINE-8]]:19: note: 3 lines including whitespace and comments (threshold 0)
+// CHECK-MESSAGES: :[[@LINE-9]]:19: note: 13 statements (threshold 0)
+// CHECK-MESSAGES: :[[@LINE-10]]:19: note: 1 branches (threshold 0)
+
+void foo9() { class A { void foox() {;;} }; }
+
+void bar9() { { class A { void barx() {{;;}} }; } }
+// CHECK-MESSAGES: :[[@LINE-1]]:15: warning: compound statement exceeds recommended size/complexity thresholds [readability-compound-statement-size]
+// CHECK-MESSAGES: :[[@LINE-2]]:15: note: 3 statements (threshold 0)
+//
+// CHECK-MESSAGES: :[[@LINE-4]]:40: warning: compound statement exceeds recommended size/complexity thresholds [readability-compound-statement-size]
+// CHECK-MESSAGES: :[[@LINE-5]]:40: note: 2 statements (threshold 0)