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 @@ -29,6 +29,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" @@ -56,6 +57,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,46 @@ +//===--- 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); + void diagWithFixIt(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,121 @@ +//===--- 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 + +static StringRef WarningMessage = "static declaration in header"; + +SourceRange StaticDeclarationInHeaderCheck::getStaticSourceRange( + NamedDecl const *MatchedDecl, const SourceManager &SM) { + Token Tok; + SourceLocation Loc = MatchedDecl->getSourceRange().getBegin(); + while (Loc < MatchedDecl->getSourceRange().getEnd() && + !Lexer::getRawToken(Loc, Tok, SM, getLangOpts(), true)) { + SourceRange TokenRange(Tok.getLocation(), Tok.getEndLoc()); + StringRef SourceText = Lexer::getSourceText( + CharSourceRange::getTokenRange(TokenRange), SM, getLangOpts()); + if (SourceText == "static") { + return TokenRange; + } + Loc = Tok.getEndLoc(); + } + llvm_unreachable("Could not find 'static' token"); +} + +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::diagWithFixIt(const NamedDecl *MatchedDecl, + const SourceManager &SM) { + diag(MatchedDecl->getLocation(), WarningMessage) + << FixItHint::CreateInsertion(MatchedDecl->getSourceRange().getBegin(), + "inline") + << FixItHint::CreateRemoval(getStaticSourceRange(MatchedDecl, SM)); +} + +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(), + isInAnonymousNamespace()))) + .bind("x"), + this); + Finder->addMatcher( + varDecl(isStaticStorageClass(), isInHeaderFile(HeaderFileExtensions), + unless(anyOf(hasType(isConstQualified()), isStaticLocal(), + isStaticDataMember(), isInline(), + isInAnonymousNamespace()))) + .bind("x"), + this); +} + +void StaticDeclarationInHeaderCheck::check( + const MatchFinder::MatchResult &Result) { + const auto *MatchedDecl = Result.Nodes.getNodeAs("x"); + if (!MatchedDecl) + return; + + const SourceManager *SM = Result.SourceManager; + if (!SM) + return; + + if (llvm::isa(MatchedDecl)) + diagWithFixIt(MatchedDecl, *SM); + else if (llvm::isa(MatchedDecl)) { + if (Result.Context->getLangOpts().CPlusPlus17) + diagWithFixIt(MatchedDecl, *SM); + else + diag(MatchedDecl->getLocation(), WarningMessage); + } +} + +} // 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 @@ -110,6 +110,12 @@ Warns when lambda specify a capture default and capture ``this``. +- New :doc:`misc-static-declaration-in-header + ` check. + + Warns about ``static`` variable and function declarations at file or + namespace scope in header files. + New check aliases ^^^^^^^^^^^^^^^^^ 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 @@ -257,6 +257,7 @@ `misc-non-private-member-variables-in-classes `_, `misc-redundant-expression `_, "Yes" `misc-static-assert `_, "Yes" + `misc-static-declaration-in-header `_, "Yes" `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,59 @@ +.. 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 void f(){} + static int x = 123; + +The code should instead be changed to replace ``static`` with ``inline``, for +both function and variable declarations (since C++17): + +.. code-block:: c++ + + // foo.h + inline void f(){} + inline int x = 123; // Since C++17 + +The check will issue automatic fixes if supported. + +Alternatively, the code could be changed to remove ``static`` and move the +definitions to a separate translation unit. + +.. code-block:: c++ + + // foo.h + void f(); + int x; + + // foo.cpp + void f(){} + int x = 123; + +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,46 @@ +// RUN: %check_clang_tidy -std=c++98,c++03,c++11,c++14 %s misc-static-declaration-in-header %t +// RUN: %check_clang_tidy -check-suffix=CXX17-OR-LATER -std=c++17-or-later %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] +// CHECK-FIXES-CXX17-OR-LATER: {{^}}inline int v1 = 123;{{$}} + +static void f1(){} +// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: static declaration in header +// CHECK-FIXES: {{^}}inline void f1(){}{{$}} + +namespace foo { + static int v2 = 123; + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: static declaration in header + // CHECK-FIXES-CXX17-OR-LATER: {{^}} inline int v2 = 123;{{$}} + + static int f2(){} + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: static declaration in header + // CHECK-FIXES: {{^}} inline int f2(){}{{$}} +} + +// OK +struct Foo { + static const int v3 = 123; + static void f3(){} +}; + +// OK +void f4() { + static int v4 = 123; +} + +// OK, static doesn't make matters worse here due to the anonymous namespace +namespace { + static int v5 = 123; + static int f5(){} +} + +// OK, static is only redundant (readability) +static const int v6 = 123; +static inline void f6(){} + +#if __cplusplus >= 201103L +static constexpr int v7 = 123; +static constexpr int f7(){ return 123; } +#endif