[clang-tidy] Add old style function check

[piotrdz]

Summary

This is another patch from my tool colobot-lint.

This adds a new check misc-old-style-function, which checks for instances of functions written in legacy C style.

As before, I hope I did everything according to LLVM style, including testcases and documentation.

If time allows, I will post another patch to extend this check with FixIt hints to localize variable declarations.

[Eugene.Zelenko]

I think extern "C" functions should be kept with (void).

I may be mistaken, but looks like code doesn't check for in C++ mode.

[Eugene.Zelenko]

Just for reference:

• Previous attempt: D7639.

• Bugzilla request.

[piotrdz]

@eugene: I don't understand, what does declaring function with "void" argument have in common with this review? I only check here variable declarations inside functions.

Maybe you meant my other review for inconsistent declaration parameter names? If so, this is how it behaves currently:

$ cat test.c
void allright();
void allright(void);

void notallright(int a);
void notallright(int b);

$ clang-tidy -checks='-*,readability-inconsistent-declaration-parameter-name' test.c -- -std=c11
1 warning generated.
/work/clang-trunk/test.c:4:6: warning: function 'notallright' has other declaration with different parameter name(s) [readability-inconsistent-declaration-parameter-name]
void notallright(int a);
     ^
/work/clang-trunk/test.c:5:6: note: other declaration seen here
void notallright(int b);

So I see no reason to add specific checks for functions with "void" parameter, unless you see something wrong with this behavior?

[cfe-commits]

@Aaron: Yes, I'm aware of that. I wanted to show that my check does not
take this into account.

In C++ this code is equivalent, so I think nothing should be reported,
unless we really want to get rid of that void, but I suppose this other
check does it already.

And when it comes to C, in well-formed C code, we should never see those
two definitions of allright() and allright(void), as they are
declarations of different functions. If I try to add empty bodies {} to
define them, this example won't even compile.

So anyway, I still don't understand the problem here, unless somebody
explains it in clearer terms.

Best regards,
Piotr Dziwinski

[Eugene.Zelenko]

Sorry, I was mislead by check name.

[alexfh]

A high-level comment:

It seems that the scope of the check is artificially made too narrow. The check could actually look at any POD variables declared unnecessarily far from their initialization and usages. And here the value of the check would also be much higher, if it can automatically fix the code.

I'll review the code more thoroughly later.

[alexfh]

Apparently, I forgot to submit the comments a looong time ago. Sorry for the delay.

In D12473#236401, @alexfh wrote:

A high-level comment:

It seems that the scope of the check is artificially made too narrow. The check could actually look at any POD variables declared unnecessarily far from their initialization and usages. And here the value of the check would also be much higher, if it can automatically fix the code.

I'll review the code more thoroughly later.

It looks like addressing this comment will significantly change the code, so a more thorough review should follow that step.

A few initial comments though.

clang-tidy/misc/OldStyleFunctionCheck.cpp

29

I think, this class could be replaced with an AST matcher (functionDecl(forEachDescendant(stmt( ... )))) that you could run using the clang::ast_matchers::match() function. Where the ... part would contain the matcher-based implementation of the isInteresting method.

The result would be a more local and clear code that expresses the constraints on the AST nodes you're interested in.

76

80 column limit is violated here and in a few other places. Consider clang-format'ting the code.

83

This kind of a comment is not common in llvm/clang code.

149

I'd use the if (T* x = ...) ... style for this and the next condition.

171

raw_string_ostream is a more common way to format text, and it should be more effective.

docs/clang-tidy/checks/misc-old-style-function.rst

25

Please add a newline at the end of file.

[piotrdz]

@alexfh: Ah, I forgot about this review. I will mark it as abandoned, because I have already started work on new check for localizing variables. It will have the wider scope that Eugene proposed originally, that is to move variable declarations closer to their first use. If you're curious how the code looks, you can see it on my Github fork: https://github.com/piotrdz/clang-tools-extra, as I said in discussion on cfe-dev some time ago. I will submit this new check in a new review once I finish it.

clang-tidy/misc/CMakeLists.txt

   MiscTidyModule.cpp
   MoveConstructorInitCheck.cpp
   NoexceptMoveConstructorCheck.cpp
+  OldStyleFunctionCheck.cpp
   StaticAssertCheck.cpp
   SwappedArgumentsCheck.cpp
   UndelegatedConstructor.cpp

clang-tidy/misc/MiscTidyModule.cpp

 #include "MacroRepeatedSideEffectsCheck.h"
 #include "MoveConstructorInitCheck.h"
 #include "NoexceptMoveConstructorCheck.h"
+#include "OldStyleFunctionCheck.h"
 #include "StaticAssertCheck.h"
 #include "SwappedArgumentsCheck.h"
 #include "UndelegatedConstructor.h"

         "misc-move-constructor-init");
     CheckFactories.registerCheck<NoexceptMoveConstructorCheck>(
         "misc-noexcept-move-constructor");
+    CheckFactories.registerCheck<OldStyleFunctionCheck>(
+        "misc-old-style-function");
     CheckFactories.registerCheck<StaticAssertCheck>(
         "misc-static-assert");
     CheckFactories.registerCheck<SwappedArgumentsCheck>(

clang-tidy/misc/OldStyleFunctionCheck.h

+//===--- OldStyleFunctionCheck.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_OLD_STYLE_FUNCTION_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_OLD_STYLE_FUNCTION_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+/// \brief Check for functions written in legacy C style
+///
+/// This check finds instances of functions, which use legacy C style
+/// of declaring all variables in a function at the beginning of function
+/// scope, for example:
+/// \code
+/// void foo()
+/// {
+///   int a, x;
+///   // after many lines in between, first use of variables:
+///   a = bar();
+///   for (i = 0; i < 10; i++) { /*...*/ }
+/// }
+/// \endcode
+///
+/// A given variable is considered written in legacy style if:
+///  - it is a local variable of POD type
+///  - its declaration does not have initialization
+///  - its declaration and first use in function are separated by more than a
+///    certain number of lines of code; this number is configured by parameter
+///    DeclarationAndFirstUseDistanceThreshold with default vaule of 3
+///
+/// FIXME: this check can be extended with FixIt hints to automatically refactor
+///        code so that variable declarations are moved to the place of first use,
+///        thus localizing the variable declaration.
+///
+class OldStyleFunctionCheck : public ClangTidyCheck {
+public:
+  OldStyleFunctionCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context),
+        DeclarationAndFirstUseDistanceThreshold(Options.get<int>("DeclarationAndFirstUseDistanceThreshold", 3))
+    {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+
+private:
+    int DeclarationAndFirstUseDistanceThreshold;
+};
+
+} // namespace misc
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_OLD_STYLE_FUNCTION_H
+

clang-tidy/misc/OldStyleFunctionCheck.cpp

+//===--- OldStyleFunctionCheck.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 "OldStyleFunctionCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/RecursiveASTVisitor.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+#include <unordered_set>
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+namespace {
+
+class OldStyleDeclarationFinder : public RecursiveASTVisitor<OldStyleDeclarationFinder> {
+public:
+  OldStyleDeclarationFinder(ASTContext* Context, int DeclarationAndFirstUseDistanceThreshold);
+
+  bool VisitStmt(Stmt* Statement);
+
+  int getOldStyleDeclarationsCount() const;
+  std::string getFewFirstOldStyleDeclarations() const;
+
+private:
+  bool isInteresting(const VarDecl* VariableDeclaration);
+  bool isUninitializedPodVariable(const VarDecl* VariableDeclaration, ASTContext* Context);
+  bool hasImplicitOrDefaultedInitialization(const VarDecl* VariableDeclaration);
+
+  ASTContext* Context;
+  int DeclarationAndFirstUseDistanceThreshold;
+
+  static constexpr int MINIMUM_SET_SIZE = 10;
+  llvm::SmallSet<StringRef, MINIMUM_SET_SIZE> OldStyleDeclarations;
+  llvm::SmallSet<StringRef, MINIMUM_SET_SIZE> CorrectStyleDeclarations;
+
+  static constexpr int MAX_NAMES_IN_DESCRIPTION = 5;
+  llvm::SmallVector<StringRef, MAX_NAMES_IN_DESCRIPTION> FewFirstOldStyleDeclarations;
+};
+
+} // anonymous namespace
+
+void OldStyleFunctionCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(functionDecl(
+                       unless(anyOf(isExpansionInSystemHeader(),
+                                    isImplicit())),
+                       isDefinition())
+                       .bind("functionDefinition"),
+                     this);
+}
+
+void OldStyleFunctionCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto* FunctionDefinition = Result.Nodes.getNodeAs<FunctionDecl>("functionDefinition");
+  auto* FunctionBody = FunctionDefinition->getBody();
+  if (FunctionDefinition == nullptr || FunctionBody == nullptr)
+    return;
+
+  OldStyleDeclarationFinder Finder(Result.Context, DeclarationAndFirstUseDistanceThreshold);
+  Finder.TraverseStmt(FunctionBody);
+
+  auto OldStyleDeclarationCount = Finder.getOldStyleDeclarationsCount();
+  if (OldStyleDeclarationCount == 0)
+    return;
+
+  diag(FunctionDefinition->getLocation(),
+       "function '%0' seems to be written in legacy C style: "
+       "it has %select{an|%1}2 uninitialized POD type variable%s1 declared far from %select{its|their}2 point of use: %3")
+      << FunctionDefinition->getQualifiedNameAsString()
+      << OldStyleDeclarationCount
+      << (OldStyleDeclarationCount == 1 ? 0 : 1)
+      << Finder.getFewFirstOldStyleDeclarations();
+}
+
+////////////////////////////
+
+OldStyleDeclarationFinder::OldStyleDeclarationFinder(ASTContext* Context, int DeclarationAndFirstUseDistanceThreshold)
+    : Context(Context), DeclarationAndFirstUseDistanceThreshold(DeclarationAndFirstUseDistanceThreshold)
+{}
+
+bool OldStyleDeclarationFinder::VisitStmt(Stmt* Statement) {
+  const auto* DeclarationRef = dyn_cast_or_null<const DeclRefExpr>(Statement);
+  if (DeclarationRef == nullptr)
+    return true;
+
+  const auto* VariableDeclaration = dyn_cast_or_null<const VarDecl>(DeclarationRef->getDecl());
+  if (! isInteresting(VariableDeclaration))
+    return true;
+
+  auto& SM = Context->getSourceManager();
+
+  auto DeclarationLineNumber = SM.getPresumedLineNumber(VariableDeclaration->getLocation());
+  auto FirstUseLineNumber = SM.getPresumedLineNumber(Statement->getLocStart());
+  auto Name = VariableDeclaration->getName();
+
+  if (FirstUseLineNumber < DeclarationLineNumber + DeclarationAndFirstUseDistanceThreshold) {
+    CorrectStyleDeclarations.insert(Name);
+  }
+  else {
+    OldStyleDeclarations.insert(Name);
+    if (FewFirstOldStyleDeclarations.size() < MAX_NAMES_IN_DESCRIPTION)
+      FewFirstOldStyleDeclarations.push_back(Name);
+  }
+
+  return true;
+}
+
+bool OldStyleDeclarationFinder::isInteresting(const VarDecl* VariableDeclaration) {
+  // we're only interested in local, POD type variables that don't have proper initialization
+  if (VariableDeclaration == nullptr ||
+      !VariableDeclaration->hasLocalStorage() ||
+      VariableDeclaration->isImplicit() ||
+      ParmVarDecl::classof(VariableDeclaration) ||
+     !isUninitializedPodVariable(VariableDeclaration, Context)) {
+    return false;
+  }
+
+  auto Name = VariableDeclaration->getName();
+
+  // already visited?
+  if (OldStyleDeclarations.count(Name) > 0 ||
+      CorrectStyleDeclarations.count(Name) > 0) {
+    return false;
+  }
+
+  return true;
+}
+
+bool OldStyleDeclarationFinder::isUninitializedPodVariable(const VarDecl* VariableDeclaration, ASTContext* Context) {
+  auto VariableType = VariableDeclaration->getType();
+  if (! VariableType.isPODType(*Context))
+    return false;
+
+  if (VariableType->isRecordType() && VariableDeclaration->hasInit())
+    return hasImplicitOrDefaultedInitialization(VariableDeclaration);
+
+  return !VariableDeclaration->hasInit();
+}
+
+bool OldStyleDeclarationFinder::hasImplicitOrDefaultedInitialization(const VarDecl* VariableDeclaration) {
+  const auto* InitConstructExpr = llvm::dyn_cast_or_null<const CXXConstructExpr>(VariableDeclaration->getInit());
+  if (InitConstructExpr == nullptr)
+    return false;
+
+  const auto* TypeConstructorDecl = InitConstructExpr->getConstructor();
+  if (TypeConstructorDecl == nullptr)
+    return false;
+
+  return TypeConstructorDecl->isImplicit() ||
+         TypeConstructorDecl->getCanonicalDecl()->isDefaulted();
+}
+
+int OldStyleDeclarationFinder::getOldStyleDeclarationsCount() const {
+  return OldStyleDeclarations.size();
+}
+
+std::string OldStyleDeclarationFinder::getFewFirstOldStyleDeclarations() const {
+  std::string result;
+
+  bool first = true;
+  for (const auto& declaration : FewFirstOldStyleDeclarations) {
+    if (!first)
+      result += ", ";
+    else
+      first = false;
+
+    result += "'";
+    result += declaration.str();
+    result += "'";
+  }
+
+  if (OldStyleDeclarations.size() > FewFirstOldStyleDeclarations.size())
+    result += "...";
+
+  return result;
+}
+
+} // namespace misc
+} // namespace tidy
+} // namespace clang
+

docs/clang-tidy/checks/list.rst

    misc-macro-repeated-side-effects
    misc-move-constructor-init
    misc-noexcept-move-constructor
+   misc-old-style-function
    misc-static-assert
    misc-swapped-arguments
    misc-undelegated-constructor

docs/clang-tidy/checks/misc-old-style-function.rst

+misc-old-style-function
+=======================
+
+
+This check finds instances of functions, which use legacy C style of declaring
+all variables in a function at the beginning of function scope.
+
+Example:
+
+.. code:: c++
+
+  void foo() {
+    int a, x;
+    // after many lines in between, first use of variables:
+    a = bar();
+    for (i = 0; i < 10; i++) { /*...*/ }
+  }
+
+A given variable is considered written in legacy style if:
+ - it is a local variable of POD type
+ - its declaration does not have initialization
+ - its declaration and first use in function are separated by more than
+   a certain number of lines of code; this number is configured by
+   parameter `DeclarationAndFirstUseDistanceThreshold` with default vaule of 3
 No newline at end of file

test/clang-tidy/misc-old-style-function.cpp

+// RUN: %python %S/check_clang_tidy.py %s misc-old-style-function %t -config="{CheckOptions: [{key: misc-old-style-function.DeclarationAndFirstUseDistanceThreshold, value: 4}]}" --
+
+int fooInt();
+
+// CHECK-MESSAGES: :[[@LINE+1]]:6: warning: function 'oldStyleFunctionOneIntVariable' seems to be written in legacy C style: it has an uninitialized POD type variable declared far from its point of use: 'x' [misc-old-style-function]
+void oldStyleFunctionOneIntVariable() {
+  int x;
+  //
+  //
+  //
+  x = fooInt();
+}
+
+// CHECK-MESSAGES: :[[@LINE+1]]:6: warning: function 'oldStyleFunctionTwoIntVariables' seems to be written in legacy C style: it has 2 uninitialized POD type variables declared far from their point of use: 'x', 'y' [misc-old-style-function]
+void oldStyleFunctionTwoIntVariables() {
+  int x, y;
+  //
+  //
+  //
+  x = fooInt();
+  y = x + 2;
+}
+
+// CHECK-MESSAGES: :[[@LINE+1]]:6: warning: function 'oldStyleFunctionManyIntVariables' seems to be written in legacy C style: it has 7 uninitialized POD type variables declared far from their point of use: 'a', 'b', 'c', 'd', 'e'... [misc-old-style-function]
+void oldStyleFunctionManyIntVariables() {
+  int a, b, c, d, e, f, g;
+  //
+  //
+  //
+  a = b = c = d = e = f = g = fooInt();
+}
+
+struct Pod {
+  int a;
+};
+
+Pod fooPod();
+
+// CHECK-MESSAGES: :[[@LINE+1]]:6: warning: function 'oldStyleFunctionWithUserDefinedPodTypeVariable' seems to be written in legacy C style: it has an uninitialized POD type variable declared far from its point of use: 'x' [misc-old-style-function]
+void oldStyleFunctionWithUserDefinedPodTypeVariable() {
+  Pod x;
+  //
+  //
+  //
+  x = fooPod();
+}
+
+void functionWithDeclarationAndFirstUseBelowThreshold() {
+  int x;
+  //
+  //
+  x = fooInt();
+}
+
+void functionWithInitializedVariables() {
+  int x = 0, y = 0;
+  //
+  //
+  //
+  x = fooInt();
+  y = x + 2;
+}
+
+struct NonPod {
+  int a = 0;
+};
+
+NonPod fooNonPod();
+
+void functionWithNonPodTypeVariables() {
+  NonPod x, y;
+  //
+  //
+  //
+  x = fooNonPod();
+  y = fooNonPod();
+}
+
+void ignoreFunctionParameters(int& x, int &y) {
+  //
+  //
+  //
+  x = fooInt();
+  y = x + 2;
+}
+