Index: clang-tidy/misc/CMakeLists.txt =================================================================== --- clang-tidy/misc/CMakeLists.txt +++ clang-tidy/misc/CMakeLists.txt @@ -12,6 +12,7 @@ MiscTidyModule.cpp MoveConstructorInitCheck.cpp NoexceptMoveConstructorCheck.cpp + OldStyleFunctionCheck.cpp StaticAssertCheck.cpp SwappedArgumentsCheck.cpp UndelegatedConstructor.cpp Index: clang-tidy/misc/MiscTidyModule.cpp =================================================================== --- clang-tidy/misc/MiscTidyModule.cpp +++ clang-tidy/misc/MiscTidyModule.cpp @@ -20,6 +20,7 @@ #include "MacroRepeatedSideEffectsCheck.h" #include "MoveConstructorInitCheck.h" #include "NoexceptMoveConstructorCheck.h" +#include "OldStyleFunctionCheck.h" #include "StaticAssertCheck.h" #include "SwappedArgumentsCheck.h" #include "UndelegatedConstructor.h" @@ -55,6 +56,8 @@ "misc-move-constructor-init"); CheckFactories.registerCheck( "misc-noexcept-move-constructor"); + CheckFactories.registerCheck( + "misc-old-style-function"); CheckFactories.registerCheck( "misc-static-assert"); CheckFactories.registerCheck( Index: clang-tidy/misc/OldStyleFunctionCheck.h =================================================================== --- clang-tidy/misc/OldStyleFunctionCheck.h +++ clang-tidy/misc/OldStyleFunctionCheck.h @@ -0,0 +1,63 @@ +//===--- 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("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 + Index: clang-tidy/misc/OldStyleFunctionCheck.cpp =================================================================== --- clang-tidy/misc/OldStyleFunctionCheck.cpp +++ clang-tidy/misc/OldStyleFunctionCheck.cpp @@ -0,0 +1,189 @@ +//===--- 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 + +using namespace clang::ast_matchers; + +namespace clang { +namespace tidy { +namespace misc { + +namespace { + +class OldStyleDeclarationFinder : public RecursiveASTVisitor { +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 OldStyleDeclarations; + llvm::SmallSet CorrectStyleDeclarations; + + static constexpr int MAX_NAMES_IN_DESCRIPTION = 5; + llvm::SmallVector 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("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(Statement); + if (DeclarationRef == nullptr) + return true; + + const auto* VariableDeclaration = dyn_cast_or_null(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(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 + Index: docs/clang-tidy/checks/list.rst =================================================================== --- docs/clang-tidy/checks/list.rst +++ docs/clang-tidy/checks/list.rst @@ -31,6 +31,7 @@ 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 Index: docs/clang-tidy/checks/misc-old-style-function.rst =================================================================== --- docs/clang-tidy/checks/misc-old-style-function.rst +++ docs/clang-tidy/checks/misc-old-style-function.rst @@ -0,0 +1,24 @@ +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 Index: test/clang-tidy/misc-old-style-function.cpp =================================================================== --- test/clang-tidy/misc-old-style-function.cpp +++ test/clang-tidy/misc-old-style-function.cpp @@ -0,0 +1,86 @@ +// 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; +} +