carlosgalvezp created this revision.
Herald added a subscriber: xazax.hun.
Herald added a reviewer: njames93.
Herald added a project: All.
carlosgalvezp requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.
Static declarations in header files is considered bad practice,
since header files are meant for sharing code, and static
leads to internal linkage. This can cause code bloat and lead
to subtle issues, for example ODR violations. Essentially, this
is as problematic as having anonymous namespaces in headers,
which we already have checks for.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D140290
Files:
clang-tools-extra/clang-tidy/misc/CMakeLists.txt
clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp
clang-tools-extra/clang-tidy/misc/StaticDeclarationInHeaderCheck.cpp
clang-tools-extra/clang-tidy/misc/StaticDeclarationInHeaderCheck.h
clang-tools-extra/docs/ReleaseNotes.rst
clang-tools-extra/docs/clang-tidy/checks/list.rst
clang-tools-extra/docs/clang-tidy/checks/misc/static-declaration-in-header.rst
clang-tools-extra/test/clang-tidy/checkers/misc/static-declaration-in-header.hpp
Index: clang-tools-extra/test/clang-tidy/checkers/misc/static-declaration-in-header.hpp
===================================================================
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/misc/static-declaration-in-header.hpp
@@ -0,0 +1,48 @@
+// 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; remove 'static' [misc-static-declaration-in-header]
+// CHECK-FIXES: {{^}}int v1 = 123;
+
+static const int c1 = 123;
+// CHECK-MESSAGES: :[[@LINE-1]]:18: warning: static declaration in header; remove 'static'
+// CHECK-FIXES: {{^}}const int c1 = 123;
+
+static constexpr int c2 = 123;
+// CHECK-MESSAGES: :[[@LINE-1]]:22: warning: static declaration in header; remove 'static'
+// CHECK-FIXES: {{^}}constexpr int c2 = 123;
+
+static void f1(){}
+// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: static declaration in header; remove 'static'
+// CHECK-FIXES: {{^}}void f1(){}
+
+namespace foo {
+ static int v2 = 123;
+ // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: static declaration in header; remove 'static'
+ // CHECK-FIXES: {{^}} int v2 = 123;
+
+ static int f2(){}
+ // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: static declaration in header; remove 'static'
+ // CHECK-FIXES: {{^}} int f2(){}
+}
+
+namespace {
+ static int v3 = 123;
+ // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: static declaration in header; remove 'static'
+ // CHECK-FIXES: {{^}} int v3 = 123;
+
+ static int f3(){}
+ // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: static declaration in header; remove 'static'
+ // CHECK-FIXES: {{^}} int f3(){}
+}
+
+// OK
+struct Foo {
+ static const int v4 = 123;
+ static void f4(){}
+};
+
+// OK
+void f5() {
+ static int v5;
+}
Index: clang-tools-extra/docs/clang-tidy/checks/misc/static-declaration-in-header.rst
===================================================================
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/misc/static-declaration-in-header.rst
@@ -0,0 +1,41 @@
+.. 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. Provides fix-it hint to remove ``static``.
+
+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<clang-tidy/checks/google/build-namespaces>`.
+
+Example of problematic code:
+
+.. code-block:: c++
+
+ // foo.h
+
+ // Bad
+ static int x = 123;
+ static void f() {}
+
+ // Good
+ int x = 123;
+ 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.
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===================================================================
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -254,6 +254,7 @@
`misc-non-private-member-variables-in-classes <misc/non-private-member-variables-in-classes.html>`_,
`misc-redundant-expression <misc/redundant-expression.html>`_, "Yes"
`misc-static-assert <misc/static-assert.html>`_, "Yes"
+ `misc-static-declaration-in-header <misc/static-declaration-in-header.html>`_, "Yes"
`misc-throw-by-value-catch-by-reference <misc/throw-by-value-catch-by-reference.html>`_,
`misc-unconventional-assign-operator <misc/unconventional-assign-operator.html>`_,
`misc-uniqueptr-reset-release <misc/uniqueptr-reset-release.html>`_, "Yes"
Index: clang-tools-extra/docs/ReleaseNotes.rst
===================================================================
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -118,6 +118,9 @@
Warns when using ``do-while`` loops.
+- New :doc:`misc-static-declaration-in-header
+ <clang-tidy/checks/misc/static-declaration-in-header>` check.
+
- New :doc:`misc-use-anonymous-namespace
<clang-tidy/checks/misc/use-anonymous-namespace>` check.
Index: clang-tools-extra/clang-tidy/misc/StaticDeclarationInHeaderCheck.h
===================================================================
--- /dev/null
+++ 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. Provides fix-it hint to remove ``static``.
+///
+/// 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
Index: clang-tools-extra/clang-tidy/misc/StaticDeclarationInHeaderCheck.cpp
===================================================================
--- /dev/null
+++ clang-tools-extra/clang-tidy/misc/StaticDeclarationInHeaderCheck.cpp
@@ -0,0 +1,103 @@
+//===--- 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_POLYMORPHIC_MATCHER(isStatic, AST_POLYMORPHIC_SUPPORTED_TYPES(FunctionDecl,
+ VarDecl)) {
+ return Node.getStorageClass() == SC_Static;
+}
+
+AST_MATCHER(FunctionDecl, isMemberFunction) {
+ return llvm::isa<CXXMethodDecl>(&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;
+ }
+}
+
+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");
+}
+
+void StaticDeclarationInHeaderCheck::storeOptions(
+ ClangTidyOptions::OptionMap &Opts) {
+ Options.store(Opts, "HeaderFileExtensions", RawStringHeaderFileExtensions);
+}
+
+void StaticDeclarationInHeaderCheck::registerMatchers(MatchFinder *Finder) {
+ Finder->addMatcher(functionDecl(isStatic(),
+ isInHeaderFile(HeaderFileExtensions),
+ unless(isMemberFunction()))
+ .bind("x"),
+ this);
+ Finder->addMatcher(
+ varDecl(isStatic(), isInHeaderFile(HeaderFileExtensions),
+ unless(anyOf(isStaticLocal(), isStaticDataMember())))
+ .bind("x"),
+ this);
+}
+
+void StaticDeclarationInHeaderCheck::check(
+ const MatchFinder::MatchResult &Result) {
+ if (const auto *MatchedDecl = Result.Nodes.getNodeAs<NamedDecl>("x")) {
+ diag(MatchedDecl->getLocation(),
+ "static declaration in header; remove 'static'")
+ << FixItHint::CreateRemoval(
+ getStaticSourceRange(MatchedDecl, *Result.SourceManager));
+ }
+}
+
+} // namespace misc
+} // namespace tidy
+} // namespace clang
Index: clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp
===================================================================
--- clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp
+++ 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<RedundantExpressionCheck>(
"misc-redundant-expression");
CheckFactories.registerCheck<StaticAssertCheck>("misc-static-assert");
+ CheckFactories.registerCheck<StaticDeclarationInHeaderCheck>(
+ "misc-static-declaration-in-header");
CheckFactories.registerCheck<ThrowByValueCatchByReferenceCheck>(
"misc-throw-by-value-catch-by-reference");
CheckFactories.registerCheck<UnconventionalAssignOperatorCheck>(
Index: clang-tools-extra/clang-tidy/misc/CMakeLists.txt
===================================================================
--- clang-tools-extra/clang-tidy/misc/CMakeLists.txt
+++ 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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
