diff --git a/clang-tools-extra/clang-tidy/modernize/CMakeLists.txt b/clang-tools-extra/clang-tidy/modernize/CMakeLists.txt --- a/clang-tools-extra/clang-tidy/modernize/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/modernize/CMakeLists.txt @@ -30,6 +30,7 @@ UseEmplaceCheck.cpp UseEqualsDefaultCheck.cpp UseEqualsDeleteCheck.cpp + UseInlineConstVariablesInHeadersCheck.cpp UseNodiscardCheck.cpp UseNoexceptCheck.cpp UseNullptrCheck.cpp diff --git a/clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp b/clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp --- a/clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp @@ -32,6 +32,7 @@ #include "UseEmplaceCheck.h" #include "UseEqualsDefaultCheck.h" #include "UseEqualsDeleteCheck.h" +#include "UseInlineConstVariablesInHeadersCheck.h" #include "UseNodiscardCheck.h" #include "UseNoexceptCheck.h" #include "UseNullptrCheck.h" @@ -86,6 +87,8 @@ CheckFactories.registerCheck("modernize-use-equals-default"); CheckFactories.registerCheck( "modernize-use-equals-delete"); + CheckFactories.registerCheck( + "modernize-use-inline-const-variables-in-headers"); CheckFactories.registerCheck( "modernize-use-nodiscard"); CheckFactories.registerCheck("modernize-use-noexcept"); diff --git a/clang-tools-extra/clang-tidy/modernize/UseInlineConstVariablesInHeadersCheck.h b/clang-tools-extra/clang-tidy/modernize/UseInlineConstVariablesInHeadersCheck.h new file mode 100644 --- /dev/null +++ b/clang-tools-extra/clang-tidy/modernize/UseInlineConstVariablesInHeadersCheck.h @@ -0,0 +1,57 @@ +//===--- UseInlineConstVariablesInHeadersCheck.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_MODERNIZE_USEINLINECONSTVARIABLESINHEADERSCHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_USEINLINECONSTVARIABLESINHEADERSCHECK_H + +#include "../ClangTidyCheck.h" +#include "../utils/FileExtensionsUtils.h" + +namespace clang { +namespace tidy { +namespace modernize { + +/// Finds non-extern non-inline function and variable definitions in header +/// files, which can lead to potential ODR violations. +/// +/// The check supports these options: +/// - `HeaderFileExtensions`: a semicolon-separated list of filename +/// extensions of header files (The filename extension should not contain +/// "." prefix). ";h;hh;hpp;hxx" by default. +/// +/// For extension-less header files, using an empty string or leaving an +/// empty string between ";" if there are other filename extensions. +/// - `CheckNonInline`: Whether to check and fix non-inline const variables +/// definitions in header files. True by default. +/// - `CheckExtern`: Whether to check extern const variables +/// declarations in header files. True by default. +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-inline-const-variables-in-headers.html +class UseInlineConstVariablesInHeadersCheck : public ClangTidyCheck { +public: + UseInlineConstVariablesInHeadersCheck(StringRef Name, + ClangTidyContext *Context); + bool isLanguageVersionSupported(const LangOptions &LangOpts) const override { + return LangOpts.CPlusPlus17; + } + void storeOptions(ClangTidyOptions::OptionMap &Opts) override; + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; + +private: + const std::string RawStringHeaderFileExtensions; + const bool CheckNonInline; + const bool CheckExtern; + utils::FileExtensionsSet HeaderFileExtensions; +}; + +} // namespace modernize +} // namespace tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_USEINLINECONSTVARIABLESINHEADERSCHECK_H diff --git a/clang-tools-extra/clang-tidy/modernize/UseInlineConstVariablesInHeadersCheck.cpp b/clang-tools-extra/clang-tidy/modernize/UseInlineConstVariablesInHeadersCheck.cpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/clang-tidy/modernize/UseInlineConstVariablesInHeadersCheck.cpp @@ -0,0 +1,107 @@ +//===--- UseInlineConstVariablesInHeadersCheck.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 "UseInlineConstVariablesInHeadersCheck.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" + +using namespace clang::ast_matchers; + +namespace clang { +namespace tidy { +namespace modernize { + +namespace { + +AST_MATCHER(NamedDecl, isInAnonymousNamespace) { + return Node.isInAnonymousNamespace(); +} + +AST_MATCHER(VarDecl, isVarInline) { return Node.isInline(); } + +AST_MATCHER(VarDecl, isTemplateVariable) { + return Node.isTemplated() || isa(Node); +} + +AST_MATCHER(VarDecl, isExternallyVisible) { return Node.isExternallyVisible(); } + +} // namespace + +UseInlineConstVariablesInHeadersCheck::UseInlineConstVariablesInHeadersCheck( + StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context), + RawStringHeaderFileExtensions(Options.getLocalOrGlobal( + "HeaderFileExtensions", utils::defaultHeaderFileExtensions())), + CheckNonInline(Options.getLocalOrGlobal("CheckNonInline", true)), + CheckExtern(Options.getLocalOrGlobal("CheckExtern", true)) { + if (!utils::parseFileExtensions(RawStringHeaderFileExtensions, + HeaderFileExtensions, + utils::defaultFileExtensionDelimiters())) { + this->configurationDiag("Invalid header file extension: '%0'") + << RawStringHeaderFileExtensions; + } +} + +void UseInlineConstVariablesInHeadersCheck::storeOptions( + ClangTidyOptions::OptionMap &Opts) { + Options.store(Opts, "HeaderFileExtensions", RawStringHeaderFileExtensions); + Options.store(Opts, "CheckNonInline", CheckNonInline); + Options.store(Opts, "CheckExtern", CheckExtern); +} + +void UseInlineConstVariablesInHeadersCheck::registerMatchers( + MatchFinder *Finder) { + const auto NonInlineConstVarDecl = varDecl( + hasGlobalStorage(), + hasDeclContext( + anyOf(translationUnitDecl(), namespaceDecl())), // is at file scope + hasType(isConstQualified()), // const-qualified + unless(hasType(isVolatileQualified())), // non-volatile + unless(isTemplateVariable()), // non-template + unless(isVarInline()), // non-inline + unless(isExternC()), // not "extern C" variable + unless(isInAnonymousNamespace())); // not within an anonymous namespace + + if (CheckNonInline) + Finder->addMatcher(varDecl(NonInlineConstVarDecl, isDefinition(), + unless(isExternallyVisible())) + .bind("non-inline-var-definition"), + this); + if (CheckExtern) + Finder->addMatcher(varDecl(NonInlineConstVarDecl, isExternallyVisible()) + .bind("extern-var-declaration"), + this); +} + +void UseInlineConstVariablesInHeadersCheck::check( + const MatchFinder::MatchResult &Result) { + const VarDecl *D = nullptr; + StringRef Msg; + bool InsertInlineKeyword = false; + + if ((D = Result.Nodes.getNodeAs("non-inline-var-definition"))) { + Msg = "global constant %0 should be marked as 'inline'"; + InsertInlineKeyword = true; + } else { + D = Result.Nodes.getNodeAs("extern-var-declaration"); + Msg = "global constant %0 should be converted to C++17 'inline variable'"; + } + + // Ignore if it comes not from a header + if (!utils::isSpellingLocInHeaderFile(D->getBeginLoc(), *Result.SourceManager, + HeaderFileExtensions)) + return; + + DiagnosticBuilder Diag = diag(D->getLocation(), Msg) << D; + if (InsertInlineKeyword) + Diag << FixItHint::CreateInsertion(D->getBeginLoc(), "inline "); +} + +} // namespace modernize +} // 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 @@ Reports identifier with unicode right-to-left characters. +- New :doc:`modernize-use-inline-const-variables-in-headers + ` check. + + Suggests switching to C++17 ``inline variables`` for non-inline const + variables definitions and extern const variables declarations in header files. + - New :doc:`readability-container-data-pointer ` 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 @@ -250,6 +250,7 @@ `modernize-use-emplace `_, "Yes" `modernize-use-equals-default `_, "Yes" `modernize-use-equals-delete `_, "Yes" + `modernize-use-inline-const-variables-in-headers `_, "Yes" `modernize-use-nodiscard `_, "Yes" `modernize-use-noexcept `_, "Yes" `modernize-use-nullptr `_, "Yes" diff --git a/clang-tools-extra/docs/clang-tidy/checks/modernize-use-inline-const-variables-in-headers.rst b/clang-tools-extra/docs/clang-tidy/checks/modernize-use-inline-const-variables-in-headers.rst new file mode 100644 --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/modernize-use-inline-const-variables-in-headers.rst @@ -0,0 +1,67 @@ +.. title:: clang-tidy - modernize-use-inline-const-variables-in-headers + +modernize-use-inline-const-variables-in-headers +=============================================== + +Finds non-inline const variables definitions and extern const variables +declarations in header in header files. Non-inline const variables make +a separate instance of the variable for each translation unit that +includes the header, which can lead to subtle violation of the ODR. +Extern const variables is a deprecated way to define a constant since C++17. + +.. code-block:: c++ + + // Foo.h + const int ConstFoo = 1; // Warning: should be marked as 'inline' + + namespace N { + constexpr int NamespaceFoo = 1; // Warning: should be marked as 'inline' + } + + extern const int ExternFoo; // Warning: should be converted to C++17 'inline variable' + + struct S { + static const int StructFoo = 1; // OK: the variable is not at file scope + }; + + int NonConstFoo = 1; // No warning: non-const global variables have external linkage + + const volatile int VolatileFoo = 1; // No warning: volatile global variables have external linkage + + template + const auto TemplateFoo = sizeof(T); // OK: templates and their instantiations have external linkage + + inline const int InlineFoo = 1; // no warning: already fixed + + // No warning: C has no 'inline variables' + extern "C" { + const int CFoo0 = 1; + } + extern "C" const int CFoo1 = 1; + + // No warning: 'inline' is invisible when within an unnamed namespace + namespace { + const int AnonNamespaceFoo = 1; + } + +Options +------- + +.. option:: HeaderFileExtensions + + A comma-separated list of filename extensions of header files (the filename + extensions should not include "." prefix). Default is "h,hh,hpp,hxx". + For header files without an extension, use an empty string (if there are no + other desired extensions) or leave an empty element in the list. E.g., + "h,hh,hpp,hxx," (note the trailing comma). + +.. option:: CheckNonInline + + When `true`, the check will find non-inline const variable definitions in + header files and suggest fixes for them. Default is `true`. + +.. option:: CheckExtern + + When `true`, the check will find extern const variable declarations in + header files. But the checker won't fix it as the definition may be located + in another translation unit. Default is `true`. diff --git a/clang-tools-extra/test/clang-tidy/checkers/modernize-use-inline-const-variables-in-headers.hpp b/clang-tools-extra/test/clang-tidy/checkers/modernize-use-inline-const-variables-in-headers.hpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/modernize-use-inline-const-variables-in-headers.hpp @@ -0,0 +1,43 @@ +// RUN: %check_clang_tidy %s -std=c++17 modernize-use-inline-const-variables-in-headers %t + +const int ConstFoo = 1; +// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: global constant 'ConstFoo' should be marked as 'inline' [modernize-use-inline-const-variables-in-headers] +// CHECK-FIXES: {{^}}inline const int ConstFoo = 1;{{$}} + +namespace N { +constexpr int NamespaceFoo = 1; +// CHECK-MESSAGES: :[[@LINE-1]]:15: warning: global constant 'NamespaceFoo' should be marked as 'inline' [modernize-use-inline-const-variables-in-headers] +// CHECK-FIXES: {{^}}inline constexpr int NamespaceFoo = 1;{{$}} +} // namespace N + +extern const int ExternFoo; +// CHECK-MESSAGES: :[[@LINE-1]]:18: warning: global constant 'ExternFoo' should be converted to C++17 'inline variable' [modernize-use-inline-const-variables-in-headers] + +struct S { + // no warning: the variable is not at file scope + static const int StructFoo = 1; +}; + +// no warning: non-const global variables have external linkage +int NonConstFoo = 1; + +// no warning: volatile global variables have external linkage +const volatile int VolatileFoo = 1; + +// no warning: templates and their instantiations have external linkage +template +const auto TemplateFoo = sizeof(T); + +// no warning: already fixed +inline const int InlineFoo = 1; + +// no warning: C has no 'inline variables' +extern "C" { +const int CFoo0 = 1; +} +extern "C" const int CFoo1 = 1; + +// no warning: 'inline' is invisible when within an unnamed namespace +namespace { +const int AnonNamespaceFoo = 1; +} // namespace