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 @@ -32,6 +32,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 @@ -33,6 +33,7 @@ #include "UseEmplaceCheck.h" #include "UseEqualsDefaultCheck.h" #include "UseEqualsDeleteCheck.h" +#include "UseInlineConstVariablesInHeadersCheck.h" #include "UseNodiscardCheck.h" #include "UseNoexceptCheck.h" #include "UseNullptrCheck.h" @@ -88,6 +89,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,55 @@ +//===--- 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 { + +/// Suggests switching to C++17 ``inline variables`` for non-inline const +/// variable definitions and extern const variable declarations in header files. +/// +/// 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 variable +/// definitions in header files. True by default. +/// - `CheckExtern`: Whether to check extern const variable +/// declarations in header files. True by default. +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, 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) { + auto NonInlineConstVarDecl = + varDecl(hasGlobalStorage(), + hasDeclContext(anyOf(translationUnitDecl(), + namespaceDecl())), // is at file scope + hasType(isConstQualified()), // const-qualified + unless(anyOf( + isInAnonymousNamespace(), // not within an anonymous namespace + isTemplateVariable(), // non-template + isInline(), // non-inline + hasType(isVolatileQualified()), // non-volatile + isExternC() // not "extern C" variable + ))); + + 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 @@ -115,6 +115,12 @@ Warns when using ``do-while`` loops. +- New :doc:`modernize-use-inline-const-variables-in-headers + ` check. + + Suggests switching to C++17 ``inline variables`` for non-inline const + variable definitions and extern const variable declarations 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 @@ -283,6 +283,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,69 @@ +.. title:: clang-tidy - modernize-use-inline-const-variables-in-headers + +modernize-use-inline-const-variables-in-headers +=============================================== + + +Suggests switching to C++17 ``inline variables`` for non-inline const +variable definitions and extern const variable declarations 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 are 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