carlosgalvezp updated this revision to Diff 485396.
carlosgalvezp added a comment.
Narrow warnings further, and provide fix-its.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D140290/new/
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,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
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,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<clang-tidy/checks/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.
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,12 @@
Warns when using ``do-while`` loops.
+- New :doc:`misc-static-declaration-in-header
+ <clang-tidy/checks/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
<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,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
Index: clang-tools-extra/clang-tidy/misc/StaticDeclarationInHeaderCheck.cpp
===================================================================
--- /dev/null
+++ 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<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
+
+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<NamedDecl>("x");
+ if (!MatchedDecl)
+ return;
+
+ const SourceManager *SM = Result.SourceManager;
+ if (!SM)
+ return;
+
+ if (llvm::isa<FunctionDecl>(MatchedDecl))
+ diagWithFixIt(MatchedDecl, *SM);
+ else if (llvm::isa<VarDecl>(MatchedDecl)) {
+ if (Result.Context->getLangOpts().CPlusPlus17)
+ diagWithFixIt(MatchedDecl, *SM);
+ else
+ diag(MatchedDecl->getLocation(), WarningMessage);
+ }
+}
+
+} // 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
