[clang-tidy] Add a checker for long functions.

[bkramer]

Summary

As this is very dependent on the code base it has some ways of configuration.
It's possible to pick between 3 modes of operation:

• Line counting: number of lines including whitespace and comments
• Statement counting: number of statements within compoundStmts.
• Cyclomatic complexity: calculated from the CFG.

In addition a threshold can be picked, warnings are only emitted when it is met.
Sadly this is not configurable at runtime yet as there is some plumbing missing.

[alexfh]

I wonder if any style guides define function size threshold in terms of cyclomatic complexity? Is it actually useful as a warning? It may be interesting to developers looking for a place in their code to start refactoring from, but this task requires a bit different representation of the results - tables or charts; diagnostic messages are sub-optimal here. I also think, that at the point you start needing a CFG, the check becomes closer to the static analyzer realm, as it provides more useful tools for path-based analysis.

[bkramer]

In D4986#4, @alexfh wrote:

I wonder if any style guides define function size threshold in terms of cyclomatic complexity? Is it actually useful as a warning?

It doesn't seem to be very common in C++ coding styles (probably due to lack of tools). I've seen warnings for this in other languages, e.g. Microsoft's C# analyzer puts an upper bound on cyclomatic complexity. It's yet another metric that seems slightly more useful than counting lines or statements.

I couldn't fully make up my mind about the static analyzer as its goals are slightly different. It tries hard to prune dead paths to avoid false positives while we still want to warn about complex functions that are mostly dead. Not sure if that matters in practice though.

This checker is a bit of a grab bag of metrics I could think of. It needs some configuration from the user to be useful.

[bkramer]

• Replace CFG construction with a simple branch counter as suggested by Manuel.

[alexfh]

Counting control statements is much more straightforward, thanks. A few comments inline.

clang-tidy/misc/FunctionLength.cpp

19 ↗

(On Diff #12903)

Move it to ASTMatchers.h? It's used in enough places already. IIUC, Manuel considers this a good enough way to filter out matches in template instantiations.

44 ↗

(On Diff #12903)

I wonder if you see specific reasons to use presumed locations here?

65 ↗

(On Diff #12903)

If someone decides to configure several thresholds, they will get duplicate "function is very large" warnings. I'd make a single "function is very large" (or maybe "function exceeds recommended size/complexity thresholds") warning, and add specific metrics as notes. Also please insert the function name to the message to make it obvious what it complains about.

unittests/clang-tidy/FunctionLengthCheckTest.cpp

1 ↗

(On Diff #12903)

It would be nice to test: multiple functions, lambdas, local classes/functions.

10 ↗

(On Diff #12903)

I'd make this function a bit more generic (e.g. return llvm::ErrorOr<std::vector<std::string>>) and move it to ClangTidyTest.h. Or maybe even extend runCheckOnCode.

[bkramer]

Use upstreamed astmatcher.
emit messages as notes.
moar tests.
minor fixes.

[alexfh]

unittests/clang-tidy/FunctionLengthCheckTest.cpp

1 ↗

(On Diff #12903)

It would be nice to test: multiple functions, lambdas, local classes/functions.

What's with this?

10 ↗

(On Diff #12903)

I'd make this function a bit more generic (e.g. return
llvm::ErrorOr<std::vector<std::string>>) and move it to ClangTidyTest.h. Or
maybe even extend runCheckOnCode.

So what's with this? And even more: wouldn't it be shorter/cleaner with lit-based tests?

[bkramer]

• Added configurability via ClangTidyOptions.
• Turned the unit test into a lit test.

[alexfh]

clang-tidy/misc/FunctionSize.cpp

21 ↗

(On Diff #13637)

Please make it 800U to instantiate get<unsigned>().

clang-tidy/tool/ClangTidyMain.cpp

108 ↗

(On Diff #13637)

1. Please move configuration-related changes to a separate patch.
2. Please break the strings after \n's (and maybe file a bug against clang-format to respect \n's located close to the column limit).
3. s/Specifing/Specifying/
4. It seems reasonable to allow inline configurations like in clang-format, if the -config= value starts with a '{':

clang-tidy -config='{CheckOptions: [{key: misc-function-size.LineThreshold, value: 0}, {key: misc-function-size.StatementThreshold, value: 0}]}' some-file.cpp

178 ↗

(On Diff #13637)

EffectiveOptions is only used for -list-checks and -dump-config. ClangTidy itself uses OptionsProvider, so overriding configuration file needs to be done in FileOptionsProvider. Alternatively, you can do:

OptionsProvider.reset(new DefaultOptionsProvider(GlobalOptions, EffectiveOptions));

after this. I'd probably prefer the first way.

182 ↗

(On Diff #13637)

This line doesn't do what you expect (mergeWith should probably have LLVM_ATTRIBUTE_UNUSED_RESULT).

[bkramer]

• Remove configuration changes, they aren't ready for submission and unrelated to this checker.
• Request get<unsigned> to read StatementThreshold from the config.

[alexfh]

LG.

clang-tidy/misc/FunctionSize.h

20 ↗

(On Diff #13696)

Please move private members to the end for consistency with other ClangTidy classes.

test/clang-tidy/misc-function-size.cpp

5 ↗

(On Diff #13696)

It seems reasonable to tighten the test a bit by adding the 'note:' prefix to all relevant CHECK lines and to the -implicit-check-not:

-implicit-check-not='{{warning:|error:|note:}}'
...
// CHECK: note: 1 statements (threshold 0)

53 ↗

(On Diff #13696)

For the lambda above the check seems to only warn on the function itself, but not on the lambda. I wonder what happens if you add statements inside the x() method. Will the warning only fire for bar2() or for both? I don't know which behavior is better, but it would be good to know what it does currently.

[bkramer]

test/clang-tidy/misc-function-size.cpp

53 ↗

(On Diff #13696)

We do not warn on lambdas currently but add the statements to the number of the surrounding function. For local classes we emit a warning for the function AND add the statements to the surrounding function.

Closed by commit rL217768 (authored by d0k).

clang-tools-extra/trunk/clang-tidy/misc/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyMiscModule
   ArgumentCommentCheck.cpp
   BoolPointerImplicitConversion.cpp
+  FunctionSize.cpp
   MiscTidyModule.cpp
   RedundantSmartptrGet.cpp
   SwappedArgumentsCheck.cpp
   UndelegatedConstructor.cpp
   UnusedRAII.cpp
   UseOverride.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic
   clangLex
   clangTidy
   )

clang-tools-extra/trunk/clang-tidy/misc/FunctionSize.h

+//===--- FunctionSize.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_MISC_FUNCTIONSIZE_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_FUNCTIONSIZE_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+
+/// \brief Checks for large functions based on various metrics.
+class FunctionSizeCheck : public ClangTidyCheck {
+public:
+  FunctionSizeCheck(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;
+  void onEndOfTranslationUnit() override;
+
+private:
+  struct FunctionInfo {
+    FunctionInfo() : Lines(0), Statements(0), Branches(0) {}
+    unsigned Lines;
+    unsigned Statements;
+    unsigned Branches;
+  };
+
+  const unsigned LineThreshold;
+  const unsigned StatementThreshold;
+  const unsigned BranchThreshold;
+
+  llvm::DenseMap<const FunctionDecl *, FunctionInfo> FunctionInfos;
+};
+
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_FUNCTIONSIZE_H

clang-tools-extra/trunk/clang-tidy/misc/FunctionSize.cpp

+//===--- FunctionSize.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 "FunctionSize.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+
+FunctionSizeCheck::FunctionSizeCheck(StringRef Name, ClangTidyContext *Context)
+    : ClangTidyCheck(Name, Context),
+      LineThreshold(Options.get("LineThreshold", -1U)),
+      StatementThreshold(Options.get("StatementThreshold", 800U)),
+      BranchThreshold(Options.get("BranchThreshold", -1U)) {}
+
+void FunctionSizeCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "LineThreshold", LineThreshold);
+  Options.store(Opts, "StatementThreshold", StatementThreshold);
+  Options.store(Opts, "BranchThreshold", BranchThreshold);
+}
+
+void FunctionSizeCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(
+      functionDecl(
+          unless(isInstantiated()),
+          forEachDescendant(
+              stmt(unless(compoundStmt()),
+                   hasParent(stmt(anyOf(compoundStmt(), ifStmt(),
+                                        anyOf(whileStmt(), doStmt(),
+                                              forRangeStmt(), forStmt())))))
+                  .bind("stmt"))).bind("func"),
+      this);
+}
+
+void FunctionSizeCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *Func = Result.Nodes.getNodeAs<FunctionDecl>("func");
+
+  FunctionInfo &FI = FunctionInfos[Func];
+
+  // Count the lines including whitespace and comments. Really simple.
+  if (!FI.Lines) {
+    if (const Stmt *Body = Func->getBody()) {
+      SourceManager *SM = Result.SourceManager;
+      if (SM->isWrittenInSameFile(Body->getLocStart(), Body->getLocEnd())) {
+        FI.Lines = SM->getSpellingLineNumber(Body->getLocEnd()) -
+                   SM->getSpellingLineNumber(Body->getLocStart());
+      }
+    }
+  }
+
+  const auto *Statement = Result.Nodes.getNodeAs<Stmt>("stmt");
+  ++FI.Statements;
+
+  // TODO: switch cases, gotos
+  if (isa<IfStmt>(Statement) || isa<WhileStmt>(Statement) ||
+      isa<ForStmt>(Statement) || isa<SwitchStmt>(Statement) ||
+      isa<DoStmt>(Statement) || isa<CXXForRangeStmt>(Statement))
+    ++FI.Branches;
+}
+
+void FunctionSizeCheck::onEndOfTranslationUnit() {
+  // If we're above the limit emit a warning.
+  for (const auto &P : FunctionInfos) {
+    const FunctionInfo &FI = P.second;
+    if (FI.Lines > LineThreshold || FI.Statements > StatementThreshold ||
+        FI.Branches > BranchThreshold) {
+      diag(P.first->getLocation(),
+           "function '%0' exceeds recommended size/complexity thresholds")
+          << P.first->getNameAsString();
+    }
+
+    if (FI.Lines > LineThreshold) {
+      diag(P.first->getLocation(),
+           "%0 lines including whitespace and comments (threshold %1)",
+           DiagnosticIDs::Note)
+          << FI.Lines << LineThreshold;
+    }
+
+    if (FI.Statements > StatementThreshold) {
+      diag(P.first->getLocation(), "%0 statements (threshold %1)",
+           DiagnosticIDs::Note)
+          << FI.Statements << StatementThreshold;
+    }
+
+    if (FI.Branches > BranchThreshold) {
+      diag(P.first->getLocation(), "%0 branches (threshold %1)",
+           DiagnosticIDs::Note)
+          << FI.Branches << BranchThreshold;
+    }
+  }
+
+  FunctionInfos.clear();
+}
+
+} // namespace tidy
+} // namespace clang

clang-tools-extra/trunk/clang-tidy/misc/MiscTidyModule.cpp

 //===--- MiscTidyModule.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 "ArgumentCommentCheck.h"
 #include "BoolPointerImplicitConversion.h"
+#include "FunctionSize.h"
 #include "RedundantSmartptrGet.h"
 #include "SwappedArgumentsCheck.h"
 #include "UndelegatedConstructor.h"
 #include "UnusedRAII.h"
 #include "UseOverride.h"
 
 namespace clang {
 namespace tidy {
 
 class MiscModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<ArgumentCommentCheck>("misc-argument-comment");
     CheckFactories.registerCheck<BoolPointerImplicitConversion>(
         "misc-bool-pointer-implicit-conversion");
+    CheckFactories.registerCheck<FunctionSizeCheck>("misc-function-size");
     CheckFactories.registerCheck<RedundantSmartptrGet>(
         "misc-redundant-smartptr-get");
     CheckFactories.registerCheck<SwappedArgumentsCheck>(
         "misc-swapped-arguments");
     CheckFactories.registerCheck<UndelegatedConstructorCheck>(
         "misc-undelegated-constructor");
     CheckFactories.registerCheck<UnusedRAIICheck>("misc-unused-raii");
     CheckFactories.registerCheck<UseOverride>("misc-use-override");

clang-tools-extra/trunk/test/clang-tidy/misc-function-size.cpp

+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: sed 's#// *[A-Z-][A-Z-]*:.*#//#' %s > %t/t.cpp
+// RUN: echo '{ Checks: "-*,misc-function-size", CheckOptions: [{key: misc-function-size.LineThreshold, value: 0}, {key: misc-function-size.StatementThreshold, value: 0}, {key: misc-function-size.BranchThreshold, value: 0}]}' > %t/.clang-tidy
+// RUN: clang-tidy %t/t.cpp -- -std=c++11 2>&1 | FileCheck %s -implicit-check-not='{{warning:|error:|note:}}'
+
+void foo1() {
+}
+
+void foo2() {;}
+// CHECK: warning: function 'foo2' exceeds recommended size/complexity thresholds
+// CHECK: note: 1 statements (threshold 0)
+
+void foo3() {
+;
+
+}
+// CHECK: warning: function 'foo3' exceeds recommended size/complexity thresholds
+// CHECK: note: 3 lines including whitespace and comments (threshold 0)
+// CHECK: note: 1 statements (threshold 0)
+
+void foo4(int i) { if (i) {} else; {}
+}
+// CHECK: warning: function 'foo4' exceeds recommended size/complexity thresholds
+// CHECK: note: 1 lines including whitespace and comments (threshold 0)
+// CHECK: note: 3 statements (threshold 0)
+// CHECK: note: 1 branches (threshold 0)
+
+void foo5(int i) {for(;i;)while(i)
+do;while(i);
+}
+// CHECK: warning: function 'foo5' exceeds recommended size/complexity thresholds
+// CHECK: note: 2 lines including whitespace and comments (threshold 0)
+// CHECK: note: 7 statements (threshold 0)
+// CHECK: note: 3 branches (threshold 0)
+
+template <typename T> T foo6(T i) {return i;
+}
+int x = foo6(0);
+// CHECK: warning: function 'foo6' exceeds recommended size/complexity thresholds
+// CHECK: note: 1 lines including whitespace and comments (threshold 0)
+// CHECK: note: 1 statements (threshold 0)
+
+void bar1() { [](){;;;;;;;;;;;if(1){}}();
+
+
+}
+// CHECK: warning: function 'bar1' exceeds recommended size/complexity thresholds
+// CHECK: note: 3 lines including whitespace and comments (threshold 0)
+// CHECK: note: 14 statements (threshold 0)
+// CHECK: note: 1 branches (threshold 0)
+
+void bar2() { class A { void barx() {;;} }; }
+// CHECK: warning: function 'bar2' exceeds recommended size/complexity thresholds
+// CHECK: note: 3 statements (threshold 0)
+//
+// CHECK: warning: function 'barx' exceeds recommended size/complexity thresholds
+// CHECK: note: 2 statements (threshold 0)