diff --git a/clang-tools-extra/clang-tidy/misc/CMakeLists.txt b/clang-tools-extra/clang-tidy/misc/CMakeLists.txt --- a/clang-tools-extra/clang-tidy/misc/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/misc/CMakeLists.txt @@ -41,6 +41,7 @@ NonPrivateMemberVariablesInClassesCheck.cpp RedundantExpressionCheck.cpp StaticAssertCheck.cpp + StaticDeclarationInHeaderCheck.cpp ThrowByValueCatchByReferenceCheck.cpp UnconventionalAssignOperatorCheck.cpp UniqueptrResetReleaseCheck.cpp diff --git a/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp b/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp --- a/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp @@ -21,6 +21,7 @@ #include "NonPrivateMemberVariablesInClassesCheck.h" #include "RedundantExpressionCheck.h" #include "StaticAssertCheck.h" +#include "StaticDeclarationInHeaderCheck.h" #include "ThrowByValueCatchByReferenceCheck.h" #include "UnconventionalAssignOperatorCheck.h" #include "UniqueptrResetReleaseCheck.h" @@ -57,6 +58,8 @@ CheckFactories.registerCheck( "misc-redundant-expression"); CheckFactories.registerCheck("misc-static-assert"); + CheckFactories.registerCheck( + "misc-static-declaration-in-header"); CheckFactories.registerCheck( "misc-throw-by-value-catch-by-reference"); CheckFactories.registerCheck( diff --git a/clang-tools-extra/clang-tidy/misc/StaticDeclarationInHeaderCheck.h b/clang-tools-extra/clang-tidy/misc/StaticDeclarationInHeaderCheck.h new file mode 100644 --- /dev/null +++ b/clang-tools-extra/clang-tidy/misc/StaticDeclarationInHeaderCheck.h @@ -0,0 +1,45 @@ +//===--- StaticDeclarationInHeaderCheck.h - clang-tidy ----------*- C++ -*-===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_STATICDECLARATIONINHEADERCHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_STATICDECLARATIONINHEADERCHECK_H + +#include "../ClangTidyCheck.h" +#include "../utils/FileExtensionsUtils.h" + +namespace clang { +namespace tidy { +namespace misc { + +/// Warns about ``static`` variable and function declarations at file or +/// namespace scope in header files. +/// +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/misc/static-declaration-in-header.html +class StaticDeclarationInHeaderCheck : public ClangTidyCheck { +public: + StaticDeclarationInHeaderCheck(StringRef Name, ClangTidyContext *Context); + bool isLanguageVersionSupported(const LangOptions &LangOpts) const override { + return LangOpts.CPlusPlus; + } + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; + void storeOptions(ClangTidyOptions::OptionMap &Opts) override; + +private: + const StringRef RawStringHeaderFileExtensions; + utils::FileExtensionsSet HeaderFileExtensions; + SourceRange getStaticSourceRange(NamedDecl const *MatchedDecl, + const SourceManager &SM); +}; + +} // namespace misc +} // namespace tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_STATICDECLARATIONINHEADERCHECK_H diff --git a/clang-tools-extra/clang-tidy/misc/StaticDeclarationInHeaderCheck.cpp b/clang-tools-extra/clang-tidy/misc/StaticDeclarationInHeaderCheck.cpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/clang-tidy/misc/StaticDeclarationInHeaderCheck.cpp @@ -0,0 +1,78 @@ +//===--- StaticDeclarationInHeaderCheck.cpp - clang-tidy ------------------===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#include "StaticDeclarationInHeaderCheck.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" +#include "clang/Lex/Lexer.h" + +using namespace clang::ast_matchers; + +namespace clang { +namespace tidy { +namespace misc { +namespace { + +AST_MATCHER(FunctionDecl, isMemberFunction) { + return llvm::isa(&Node); +} + +AST_MATCHER(VarDecl, isStaticDataMember) { return Node.isStaticDataMember(); } + +AST_POLYMORPHIC_MATCHER_P(isInHeaderFile, + AST_POLYMORPHIC_SUPPORTED_TYPES(FunctionDecl, + VarDecl), + utils::FileExtensionsSet, HeaderFileExtensions) { + return utils::isExpansionLocInHeaderFile( + Node.getBeginLoc(), Finder->getASTContext().getSourceManager(), + HeaderFileExtensions); +} + +} // namespace + +StaticDeclarationInHeaderCheck::StaticDeclarationInHeaderCheck( + StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context), + RawStringHeaderFileExtensions(Options.getLocalOrGlobal( + "HeaderFileExtensions", utils::defaultHeaderFileExtensions())) { + if (!utils::parseFileExtensions(RawStringHeaderFileExtensions, + HeaderFileExtensions, + utils::defaultFileExtensionDelimiters())) { + this->configurationDiag("Invalid header file extension: '%0'") + << RawStringHeaderFileExtensions; + } +} + +void StaticDeclarationInHeaderCheck::storeOptions( + ClangTidyOptions::OptionMap &Opts) { + Options.store(Opts, "HeaderFileExtensions", RawStringHeaderFileExtensions); +} + +void StaticDeclarationInHeaderCheck::registerMatchers(MatchFinder *Finder) { + Finder->addMatcher( + functionDecl(isStaticStorageClass(), isInHeaderFile(HeaderFileExtensions), + unless(anyOf(isMemberFunction(), isInline(), isConstexpr()))) + .bind("x"), + this); + Finder->addMatcher( + varDecl(isStaticStorageClass(), isInHeaderFile(HeaderFileExtensions), + unless(anyOf(hasType(isConstQualified()), isStaticLocal(), + isStaticDataMember()))) + .bind("x"), + this); +} + +void StaticDeclarationInHeaderCheck::check( + const MatchFinder::MatchResult &Result) { + if (const auto *MatchedDecl = Result.Nodes.getNodeAs("x")) + diag(MatchedDecl->getLocation(), "static declaration in header"); +} + +} // namespace misc +} // namespace tidy +} // namespace clang diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -118,6 +118,12 @@ Warns when using ``do-while`` loops. +- New :doc:`misc-static-declaration-in-header + ` check. + + Warns about ``static`` variable and function declarations at file or + namespace scope in header files. + - New :doc:`misc-use-anonymous-namespace ` check. diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -254,6 +254,7 @@ `misc-non-private-member-variables-in-classes `_, `misc-redundant-expression `_, "Yes" `misc-static-assert `_, "Yes" + `misc-static-declaration-in-header `_, `misc-throw-by-value-catch-by-reference `_, `misc-unconventional-assign-operator `_, `misc-uniqueptr-reset-release `_, "Yes" diff --git a/clang-tools-extra/docs/clang-tidy/checks/misc/static-declaration-in-header.rst b/clang-tools-extra/docs/clang-tidy/checks/misc/static-declaration-in-header.rst new file mode 100644 --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/misc/static-declaration-in-header.rst @@ -0,0 +1,57 @@ +.. title:: clang-tidy - misc-static-declaration-in-header + +misc-static-declaration-in-header +================================= + +Warns about ``static`` variable and function declarations at file or namespace +scope in header files. + +Header files are used to share code among different translation units. ``static``, +on the other hand, can be used to express that a given declaration has internal +linkage. Using ``static`` in header files leads to each translation unit creating +their own internal copy of the function or variable at hand, which is problematic. +This can cause unexpected results, bloat the resulting executable or even trigger +one-definition-rule (ODR) violations. + +This problem is similar to having anonymous namespaces in header files, already +covered by :doc:`google-build-namespaces`. + +Example of problematic code: + +.. code-block:: c++ + + // foo.h + static int x = 123; + static void f(){} + +The code should instead be changed to remove ``static`` from function and variable +declarations in the header, and provide their definitions in a separate translation +unit. + +.. code-block:: c++ + + // foo.h + int x; + void f(); + + // foo.cpp + int x = 123; + void f(){} + +Alternatively, functions (and variables in C++17) can be made ``inline``. + +.. code-block:: c++ + + // foo.h + inline int x = 123; // Since C++17 + inline void f(){} + +Options +------- + +.. option:: HeaderFileExtensions + + A semicolon-separated list of filename extensions of header files (the filename + extensions should not include `.` prefix). Default is `;h;hh;hpp;hxx`. + For extension-less header files, using an empty string or leaving an + empty string between `;` if there are other filename extensions. diff --git a/clang-tools-extra/test/clang-tidy/checkers/misc/static-declaration-in-header.hpp b/clang-tools-extra/test/clang-tidy/checkers/misc/static-declaration-in-header.hpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/misc/static-declaration-in-header.hpp @@ -0,0 +1,40 @@ +// RUN: %check_clang_tidy %s misc-static-declaration-in-header %t + +static int v1 = 123; +// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: static declaration in header [misc-static-declaration-in-header] + +static void f1(){} +// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: static declaration in header + +namespace foo { + static int v2 = 123; + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: static declaration in header + + static int f2(){} + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: static declaration in header +} + +namespace { + static int v3 = 123; + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: static declaration in header + + static int f3(){} + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: static declaration in header +} + +// OK +struct Foo { + static const int v4 = 123; + static void f4(){} +}; + +// OK +void f5() { + static int v5 = 123; +} + +// OK, 'static' is only redundant (readability) +static const int v6 = 123; +static constexpr int v7 = 123; +static constexpr int f6(){ return 123; } +static inline void f7(){}