piotrdz created this revision.
piotrdz added a subscriber: cfe-commits.
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.
http://reviews.llvm.org/D12473
Files:
clang-tidy/misc/CMakeLists.txt
clang-tidy/misc/MiscTidyModule.cpp
clang-tidy/misc/OldStyleFunctionCheck.cpp
clang-tidy/misc/OldStyleFunctionCheck.h
docs/clang-tidy/checks/list.rst
docs/clang-tidy/checks/misc-old-style-function.rst
test/clang-tidy/misc-old-style-function.cpp
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;
+}
+
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: 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: 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<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
+
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 <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
+
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<NoexceptMoveConstructorCheck>(
"misc-noexcept-move-constructor");
+ CheckFactories.registerCheck<OldStyleFunctionCheck>(
+ "misc-old-style-function");
CheckFactories.registerCheck<StaticAssertCheck>(
"misc-static-assert");
CheckFactories.registerCheck<SwappedArgumentsCheck>(
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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
