[clang-tidy] Option to ignore anonymous namespaces in avoid-non-const-global-variables (#93827)

Add an option to ignore warnings for cppcoreguidelines
avoid-non-const-global-variables.

Understandably, the core guidelines discourage non const global
variables, even at the TU level (see
https://github.com/isocpp/CppCoreGuidelines/issues/2195). However,
having a small TU with an interface that uses a non const variable from
an anonymous namespace can be a valid choice.

This adds an option that disables the warning just for anonymous
namespaces, i.e. at the file level. The default is still to show a
warning, just as before.

clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.cpp

@@ -7,7 +7,6 @@
 //===----------------------------------------------------------------------===//
 
 #include "AvoidNonConstGlobalVariablesCheck.h"
-#include "clang/AST/ASTContext.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/ASTMatchers/ASTMatchers.h"
 
@@ -15,13 +14,23 @@ using namespace clang::ast_matchers;
 
 namespace clang::tidy::cppcoreguidelines {
 
+AvoidNonConstGlobalVariablesCheck::AvoidNonConstGlobalVariablesCheck(
+    StringRef Name, ClangTidyContext *Context)
+    : ClangTidyCheck(Name, Context),
+      AllowInternalLinkage(Options.get("AllowInternalLinkage", false)) {}
+
 void AvoidNonConstGlobalVariablesCheck::registerMatchers(MatchFinder *Finder) {
+  auto NamespaceMatcher = AllowInternalLinkage
+                              ? namespaceDecl(unless(isAnonymous()))
+                              : namespaceDecl();
   auto GlobalContext =
       varDecl(hasGlobalStorage(),
-              hasDeclContext(anyOf(namespaceDecl(), translationUnitDecl())));
+              hasDeclContext(anyOf(NamespaceMatcher, translationUnitDecl())));
 
   auto GlobalVariable = varDecl(
       GlobalContext,
+      AllowInternalLinkage ? varDecl(unless(isStaticStorageClass()))
+                           : varDecl(),
       unless(anyOf(
           isConstexpr(), hasType(isConstQualified()),
           hasType(referenceType())))); // References can't be changed, only the
@@ -43,7 +52,6 @@ void AvoidNonConstGlobalVariablesCheck::registerMatchers(MatchFinder *Finder) {
 
 void AvoidNonConstGlobalVariablesCheck::check(
     const MatchFinder::MatchResult &Result) {
-
   if (const auto *Variable =
           Result.Nodes.getNodeAs<VarDecl>("non-const_variable")) {
     diag(Variable->getLocation(), "variable %0 is non-const and globally "
@@ -63,4 +71,9 @@ void AvoidNonConstGlobalVariablesCheck::check(
   }
 }
 
+void AvoidNonConstGlobalVariablesCheck::storeOptions(
+    ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "AllowInternalLinkage", AllowInternalLinkage);
+}
+
 } // namespace clang::tidy::cppcoreguidelines

clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.h

@@ -20,10 +20,13 @@ namespace clang::tidy::cppcoreguidelines {
 /// http://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines/avoid-non-const-global-variables.html
 class AvoidNonConstGlobalVariablesCheck : public ClangTidyCheck {
 public:
-  AvoidNonConstGlobalVariablesCheck(StringRef Name, ClangTidyContext *Context)
-      : ClangTidyCheck(Name, Context) {}
+  AvoidNonConstGlobalVariablesCheck(StringRef Name, ClangTidyContext *Context);
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+  void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
+
+private:
+  const bool AllowInternalLinkage;
 };
 
 } // namespace clang::tidy::cppcoreguidelines

clang-tools-extra/docs/ReleaseNotes.rst

@@ -269,6 +269,11 @@ Changes in existing checks
   <clang-tidy/checks/bugprone/use-after-move>` check to also handle
   calls to ``std::forward``.
 
+- Improved :doc:`cppcoreguidelines-avoid-non-const-global-variables
+  <clang-tidy/checks/cppcoreguidelines/avoid-non-const-global-variables>` check
+  with a new option `AllowInternalLinkage` to disable the warning for variables
+  with internal linkage.
+
 - Improved :doc:`cppcoreguidelines-macro-usage
   <clang-tidy/checks/cppcoreguidelines/macro-usage>` check by ignoring macro with
   hash preprocessing token.

clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/avoid-non-const-global-variables.rst

@@ -41,3 +41,11 @@ The variables ``a``, ``c``, ``c_ptr1``, ``c_const_ptr`` and ``c_reference``
 will all generate warnings since they are either a non-const globally accessible
 variable, a pointer or a reference providing global access to non-const data
 or both.
+
+Options
+-------
+
+.. option:: AllowInternalLinkage
+
+   When set to `true`, static non-const variables and variables in anonymous
+   namespaces will not generate a warning. The default value is `false`.

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-non-const-global-variables.cpp

@@ -1,4 +1,6 @@
-// RUN: %check_clang_tidy %s cppcoreguidelines-avoid-non-const-global-variables %t
+// RUN: %check_clang_tidy %s -check-suffixes=,DEFAULT cppcoreguidelines-avoid-non-const-global-variables %t
+// RUN: %check_clang_tidy %s -check-suffixes=,INTERNAL-LINKAGE cppcoreguidelines-avoid-non-const-global-variables %t -- \
+// RUN: -config="{CheckOptions: {cppcoreguidelines-avoid-non-const-global-variables.AllowInternalLinkage : 'true'}}"
 
 int nonConstInt = 0;
 // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: variable 'nonConstInt' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
@@ -38,9 +40,16 @@ int function() {
 
 namespace {
 int nonConstAnonymousNamespaceInt = 0;
-// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: variable 'nonConstAnonymousNamespaceInt' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
+// CHECK-MESSAGES-DEFAULT: :[[@LINE-1]]:5: warning: variable 'nonConstAnonymousNamespaceInt' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
+// CHECK-MESSAGES-INTERNAL-LINKAGE-NOT: :[[@LINE-2]]:5: warning: variable 'nonConstAnonymousNamespaceInt' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
 } // namespace
 
+static int nonConstStaticInt = 0;
+// CHECK-MESSAGES-DEFAULT: :[[@LINE-1]]:12: warning: variable 'nonConstStaticInt' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
+// CHECK-MESSAGES-INTERNAL-LINKAGE-NOT: :[[@LINE-2]]:12: warning: variable 'nonConstStaticInt' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
+
+static const int constStaticInt = 0;
+
 class DummyClass {
 public:
   int nonConstPublicMemberVariable = 0;
@@ -126,7 +135,8 @@ const DummyEnum constNamespaceEnumInstance = DummyEnum::first;
 namespace {
 DummyEnum nonConstAnonymousNamespaceEnumInstance = DummyEnum::first;
 }
-// CHECK-MESSAGES: :[[@LINE-2]]:11: warning: variable 'nonConstAnonymousNamespaceEnumInstance' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
+// CHECK-MESSAGES-DEFAULT: :[[@LINE-2]]:11: warning: variable 'nonConstAnonymousNamespaceEnumInstance' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
+// CHECK-MESSAGES-INTERNAL-LINKAGE-NOT: :[[@LINE-2]]:11: warning: variable 'nonConstAnonymousNamespaceEnumInstance' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
 
 // CHECKING FOR NON-CONST GLOBAL STRUCT ///////////////////////////////////////
 struct DummyStruct {
@@ -169,7 +179,8 @@ const DummyStruct constNamespaceDummyStructInstance;
 namespace {
 DummyStruct nonConstAnonymousNamespaceStructInstance;
 }
-// CHECK-MESSAGES: :[[@LINE-2]]:13: warning: variable 'nonConstAnonymousNamespaceStructInstance' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
+// CHECK-MESSAGES-DEFAULT: :[[@LINE-2]]:13: warning: variable 'nonConstAnonymousNamespaceStructInstance' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
+// CHECK-MESSAGES-INTERNAL-LINKAGE-NOT: :[[@LINE-2]]:11: warning: variable 'nonConstAnonymousNamespaceEnumInstance' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
 
 // CHECKING FOR NON-CONST GLOBAL UNION ////////////////////////////////////////
 union DummyUnion {
@@ -209,7 +220,8 @@ const DummyUnion constNamespaceDummyUnionInstance = {0x0};
 namespace {
 DummyUnion nonConstAnonymousNamespaceUnionInstance = {0x0};
 }
-// CHECK-MESSAGES: :[[@LINE-2]]:12: warning: variable 'nonConstAnonymousNamespaceUnionInstance' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
+// CHECK-MESSAGES-DEFAULT: :[[@LINE-2]]:12: warning: variable 'nonConstAnonymousNamespaceUnionInstance' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
+// CHECK-MESSAGES-INTERNAL-LINKAGE-NOT: :[[@LINE-3]]:12: warning: variable 'nonConstAnonymousNamespaceUnionInstance' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
 
 // CHECKING FOR NON-CONST GLOBAL FUNCTION POINTER /////////////////////////////
 int dummyFunction() {