[clang-tidy] Add `modernize-use-inline-const-variables-in-headers` check

[Izaron]

Summary

C++17 inline variables are far more convenient compared with
extern const variables declarations, and in general are more ODR-safe and
memory efficient compared with non-inline const variables definitions.
It is a part of a number of code styles to use C++17 inline variables.
The checker would help users to follow this code style.

[Izaron]

For example, Google Code Style strongly advises C++17 inline variables. There are company guides how to use them: https://abseil.io/tips/168, https://abseil.io/tips/140. I believe other codestyles also use the feature.

P.S. If this review is eventually approved, kindly please merge the commit on my behalf =) As I don't have merge access. My name is Evgeny Shulgin and email is [e-mail]. Sorry for inconvenience!

UPD: I've got merge access, so the paragraph above is irrelevant

[Eugene.Zelenko]

clang-tools-extra/clang-tidy/modernize/UseInlineConstVariablesInHeadersCheck.h

33

I think this is excessive comment. It's regular convention.

clang-tools-extra/docs/clang-tidy/checks/modernize-use-inline-const-variables-in-headers.rst

6

Please synchronize first statement with Release Notes.

53

Please highlight option values in this section with single back-ticks.

[LegalizeAdulthood]

You've probably had this check in development for some time, but
we just updated the documentation for contributing to clang-tidy
and it might help to go over it for your new check to see if there
is anything that stands out.
https://clang.llvm.org/extra/clang-tidy/Contributing.html

[LegalizeAdulthood]

clang-tools-extra/clang-tidy/modernize/UseInlineConstVariablesInHeadersCheck.cpp

64–68

Comment: Why don't we have a variadic unless(...) matcher?

Is there any utility to reordering these unless() clauses in
order of "most likely encountered first" to get an early out
when matching?

clang-tools-extra/clang-tidy/modernize/UseInlineConstVariablesInHeadersCheck.h

19

This block comment mentions functions, but the documentation only mentions variables.

clang-tools-extra/docs/clang-tidy/checks/modernize-use-inline-const-variables-in-headers.rst

6

...const variable definitions...const variable declarations

10

...variables are a deprecated way...

[Izaron]

Fix issues

[Izaron]

clang-tools-extra/clang-tidy/modernize/UseInlineConstVariablesInHeadersCheck.cpp

64–68

Thanks, it is more convenient!

reordering these unless() clauses in order of "most likely encountered first"

I reordered them based on my own approximal estimation. We could make a benchmark to observe the impact of different orders, but it goes beyond this patch.

clang-tools-extra/clang-tidy/modernize/UseInlineConstVariablesInHeadersCheck.h

19

Oops, that's a typo: of course there are nothing abouth functions anywhere in the checker.

[Izaron]

In D118743#3290591, @LegalizeAdulthood wrote:

You've probably had this check in development for some time, but
we just updated the documentation for contributing to clang-tidy
and it might help to go over it for your new check to see if there
is anything that stands out.
https://clang.llvm.org/extra/clang-tidy/Contributing.html

Thanks, this is excellent documentation! I learned a plenty of new things.

[LegalizeAdulthood]

Clang-Tidy tests and docs have moved to module subdirectories.

Please rebase this onto main:HEAD and:

• fold your changes into the appropriate subdirs, stripping the module prefix from new files.
• make the target check-clang-extra to validate your tests
• make the target docs-clang-tools-html to validate any docs changes

[njames93]

clang-tools-extra/clang-tidy/modernize/UseInlineConstVariablesInHeadersCheck.cpp

64–68

does unless(isVarInline(), isExternC()) mean unless(isVarInline() && isExternC()) or unless(isVarInline() || isExternC())
This kind of ambiguity could be addressed with unlessAllOf and unlessAnyOf but I don't think its worth the effort.

[Izaron]

Rebased onto main (after a long long time).

Thanks to @LegalizeAdulthood for instructions!

[Eugene.Zelenko]

clang-tools-extra/docs/clang-tidy/checks/modernize/use-inline-const-variables-in-headers.rst

5 ↗

(On Diff #466660)

Excessive newline.

[Izaron]

Removed excessive newline.

Thanks to @Eugene.Zelenko!

[Izaron]

A friendly ping 🙂

[Lounarok]

I tried this patch and it's really helpful!

However I found that it warns when it comes to constexpr static variable.

Snippet: constexpr static int UNDEFINED_ERROR{0};
Warning msg: warning: global constant 'UNDEFINED_ERROR' should be marked as 'inline' [modernize-use-inline-const-variables-in-headers]
According to this, "A constexpr specifier used in a function or static data member (since C++17) declaration implies inline."

It seems it should not warn on constexpr static variable.

Just a notification up here, I'm fine with // NOLINTNEXTLINE(modernize-use-inline-const-variables-in-headers).

[Izaron]

In D118743#3965423, @Lounarok wrote:

I tried this patch and it's really helpful!

You're welcome!

According to this, "A constexpr specifier used in a function or static data member (since C++17) declaration implies inline."

That's correct, for functions and static data members.

However I found that it warns when it comes to constexpr static variable.

Snippet: constexpr static int UNDEFINED_ERROR{0};
Warning msg: warning: global constant 'UNDEFINED_ERROR' should be marked as 'inline' [modernize-use-inline-const-variables-in-headers]
According to this, "A constexpr specifier used in a function or static data member (since C++17) declaration implies inline."
It seems it should not warn on constexpr static variable.

UNDEFINED_ERROR is neither a function nor a static data member. Therefore a warning is meaningful. There is no inline implication in your case.

(data member is a variable inside a struct/class, but not a "freestanding" one)

[carlosgalvezp]

clang-tools-extra/clang-tidy/modernize/UseInlineConstVariablesInHeadersCheck.cpp

22–24

I recently added this matcher into ASTMatchers, so you can remove this custom matcher :)

clang-tools-extra/test/clang-tidy/checkers/modernize/use-inline-const-variables-in-headers.hpp

1 ↗

(On Diff #466661)

Should tests be added for the CheckNonInline and CheckExtern options?

[Lounarok]

(data member is a variable inside a struct/class, but not a "freestanding" one)

Thanks for pointing that out. I misunderstood what data member is...

[Izaron]

Use the common isInAnonymousNamespace matcher.

Split the test into two files.

Thanks to @carlosgalvezp for advices!

[Izaron]

clang-tools-extra/test/clang-tidy/checkers/modernize/use-inline-const-variables-in-headers.hpp

1 ↗

(On Diff #466661)

I decided to split the test into two files as it is done for other checkers that have multiple options.

clang-tools-extra/clang-tidy/modernize/CMakeLists.txt

add_clang_library(clangTidyModernizeModule
   ShrinkToFitCheck.cpp
   UnaryStaticAssertCheck.cpp
   UseAutoCheck.cpp
   UseBoolLiteralsCheck.cpp
   UseDefaultMemberInitCheck.cpp
   UseEmplaceCheck.cpp
   UseEqualsDefaultCheck.cpp
   UseEqualsDeleteCheck.cpp
+  UseInlineConstVariablesInHeadersCheck.cpp
   UseNodiscardCheck.cpp
   UseNoexceptCheck.cpp
   UseNullptrCheck.cpp
   UseOverrideCheck.cpp
   UseTrailingReturnTypeCheck.cpp
   UseTransparentFunctorsCheck.cpp
   UseUncaughtExceptionsCheck.cpp
   UseUsingCheck.cpp

clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp

 #include "ShrinkToFitCheck.h"
 #include "UnaryStaticAssertCheck.h"
 #include "UseAutoCheck.h"
 #include "UseBoolLiteralsCheck.h"
 #include "UseDefaultMemberInitCheck.h"
 #include "UseEmplaceCheck.h"
 #include "UseEqualsDefaultCheck.h"
 #include "UseEqualsDeleteCheck.h"
+#include "UseInlineConstVariablesInHeadersCheck.h"
 #include "UseNodiscardCheck.h"
 #include "UseNoexceptCheck.h"
 #include "UseNullptrCheck.h"
 #include "UseOverrideCheck.h"
 #include "UseTrailingReturnTypeCheck.h"
 #include "UseTransparentFunctorsCheck.h"
 #include "UseUncaughtExceptionsCheck.h"
 #include "UseUsingCheck.h"
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<UseBoolLiteralsCheck>(
         "modernize-use-bool-literals");
     CheckFactories.registerCheck<UseDefaultMemberInitCheck>(
         "modernize-use-default-member-init");
     CheckFactories.registerCheck<UseEmplaceCheck>("modernize-use-emplace");
     CheckFactories.registerCheck<UseEqualsDefaultCheck>("modernize-use-equals-default");
     CheckFactories.registerCheck<UseEqualsDeleteCheck>(
         "modernize-use-equals-delete");
+    CheckFactories.registerCheck<UseInlineConstVariablesInHeadersCheck>(
+        "modernize-use-inline-const-variables-in-headers");
     CheckFactories.registerCheck<UseNodiscardCheck>(
         "modernize-use-nodiscard");
     CheckFactories.registerCheck<UseNoexceptCheck>("modernize-use-noexcept");
     CheckFactories.registerCheck<UseNullptrCheck>("modernize-use-nullptr");
     CheckFactories.registerCheck<UseOverrideCheck>("modernize-use-override");
     CheckFactories.registerCheck<UseTrailingReturnTypeCheck>(
         "modernize-use-trailing-return-type");
     CheckFactories.registerCheck<UseTransparentFunctorsCheck>(

clang-tools-extra/clang-tidy/modernize/UseInlineConstVariablesInHeadersCheck.h

+//===--- 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

clang-tools-extra/clang-tidy/modernize/UseInlineConstVariablesInHeadersCheck.cpp

+//===--- 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<VarTemplateSpecializationDecl>(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<VarDecl>("non-inline-var-definition"))) {
+    Msg = "global constant %0 should be marked as 'inline'";
+    InsertInlineKeyword = true;
+  } else {
+    D = Result.Nodes.getNodeAs<VarDecl>("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

clang-tools-extra/docs/ReleaseNotes.rst

- New :doc:`cppcoreguidelines-virtual-class-destructor
 
   Finds virtual classes whose destructor is neither public and virtual nor
   protected and non-virtual.
 
 - New :doc:`misc-misleading-identifier <clang-tidy/checks/misc-misleading-identifier>` check.
 
   Reports identifier with unicode right-to-left characters.
 
+- New :doc:`modernize-use-inline-const-variables-in-headers
+  <clang-tidy/checks/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
   <clang-tidy/checks/readability-container-data-pointer>` check.
 
   Finds cases where code could use ``data()`` rather than the address of the
   element at index 0 in a container.
 
 - New :doc:`readability-identifier-length
   <clang-tidy/checks/readability-identifier-length>` check.

clang-tools-extra/docs/clang-tidy/checks/list.rst

.. csv-table::
    `modernize-shrink-to-fit <modernize-shrink-to-fit.html>`_, "Yes"
    `modernize-unary-static-assert <modernize-unary-static-assert.html>`_, "Yes"
    `modernize-use-auto <modernize-use-auto.html>`_, "Yes"
    `modernize-use-bool-literals <modernize-use-bool-literals.html>`_, "Yes"
    `modernize-use-default-member-init <modernize-use-default-member-init.html>`_, "Yes"
    `modernize-use-emplace <modernize-use-emplace.html>`_, "Yes"
    `modernize-use-equals-default <modernize-use-equals-default.html>`_, "Yes"
    `modernize-use-equals-delete <modernize-use-equals-delete.html>`_, "Yes"
+   `modernize-use-inline-const-variables-in-headers <modernize-use-inline-const-variables-in-headers.html>`_, "Yes"
    `modernize-use-nodiscard <modernize-use-nodiscard.html>`_, "Yes"
    `modernize-use-noexcept <modernize-use-noexcept.html>`_, "Yes"
    `modernize-use-nullptr <modernize-use-nullptr.html>`_, "Yes"
    `modernize-use-override <modernize-use-override.html>`_, "Yes"
    `modernize-use-trailing-return-type <modernize-use-trailing-return-type.html>`_, "Yes"
    `modernize-use-transparent-functors <modernize-use-transparent-functors.html>`_, "Yes"
    `modernize-use-uncaught-exceptions <modernize-use-uncaught-exceptions.html>`_, "Yes"
    `modernize-use-using <modernize-use-using.html>`_, "Yes"

clang-tools-extra/docs/clang-tidy/checks/modernize-use-inline-const-variables-in-headers.rst

+.. 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<typename T>
+   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`.

clang-tools-extra/test/clang-tidy/checkers/modernize-use-inline-const-variables-in-headers.hpp

+// 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 <typename T>
+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