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

2024-05-30 Thread Pascal Jungblut via cfe-commits 

https://github.com/pascalj created 
https://github.com/llvm/llvm-project/pull/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.

>From 89265bfcca48a879f281b9ef8eb6e0730794f985 Mon Sep 17 00:00:00 2001
From: Pascal Jungblut 
Date: Thu, 30 May 2024 14:28:50 +0200
Subject: [PATCH] Add option to ignore anonymous namespaces in
 avoid-non-const-global-variables

---
 .../AvoidNonConstGlobalVariablesCheck.cpp |   6 +-
 .../avoid-non-const-global-variables.rst  |  22 +++
 .../avoid-non-const-global-variables.cpp  | 137 --
 3 files changed, 118 insertions(+), 47 deletions(-)

diff --git 
a/clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.cpp
 
b/clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.cpp
index ee17b0e014288..b536016a3cccd 100644
--- 
a/clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.cpp
+++ 
b/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"
 
@@ -16,9 +15,12 @@ using namespace clang::ast_matchers;
 namespace clang::tidy::cppcoreguidelines {
 
 void AvoidNonConstGlobalVariablesCheck::registerMatchers(MatchFinder *Finder) {
+  auto NamespaceMatcher = Options.get("AllowAnonymousNamespace", false)
+  ? namespaceDecl(unless(isAnonymous()))
+  : namespaceDecl();
   auto GlobalContext =
   varDecl(hasGlobalStorage(),
-  hasDeclContext(anyOf(namespaceDecl(), translationUnitDecl(;
+  hasDeclContext(anyOf(NamespaceMatcher, translationUnitDecl(;
 
   auto GlobalVariable = varDecl(
   GlobalContext,
diff --git 
a/clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/avoid-non-const-global-variables.rst
 
b/clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/avoid-non-const-global-variables.rst
index 21c20af6e8107..e2350872c6ece 100644
--- 
a/clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/avoid-non-const-global-variables.rst
+++ 
b/clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/avoid-non-const-global-variables.rst
@@ -41,3 +41,25 @@ 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:: AllowAnonymousNamespace
+
+   When set to `true` (default is `false`), non-const variables in anonymous 
namespaces will not generate a warning.
+
+Example
+^^^
+
+.. code-block:: c++
+
+   namespace {
+ int counter = 0;
+   }
+
+   int Increment() {
+return ++counter;
+   }
+
+will not generate a warning for `counter` if :option:`AllowAnonymousNamespace` 
is set to `true`.
diff --git 
a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-non-const-global-variables.cpp
 
b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-non-const-global-variables.cpp
index 3ca1029433d22..8956dd414356e 100644
--- 
a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-non-const-global-variables.cpp
+++ 
b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-non-const-global-variables.cpp
@@ -1,21 +1,30 @@
-// RUN: %check_clang_tidy %s 
cppcoreguidelines-avoid-non-const-global-variables %t
+// RUN: %check_clang_tidy %s -check-suffix=DEFAULT 
cppcoreguidelines-avoid-non-const-global-variables %t
+// RUN: %check_clang_tidy %s -check-suffix=NAMESPACE 
cppcoreguidelines-avoid-non-const-global-variables %t -- \
+// RUN: -config="{CheckOptions: 
{cppcoreguidelines-avoid-non-const-global-variables.AllowAnonymousNamespace : 
'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]
+// CHECK-MESSAGES-DEFAULT: [[@LINE-1]]:5: warning: variable 'nonConstInt' is 
non-const and globally accessible, consider making it const 
[cppcoreguidelines-avoid-non-const-global-variables]
+// CHECK-MESSAGES-NAMESPACE: [[@LINE-2]]:5: warning: variable 'nonConstInt' is 
non-const

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

2024-05-31 Thread Pascal Jungblut via cfe-commits 

https://github.com/pascalj updated 
https://github.com/llvm/llvm-project/pull/93827

>From abe862b84fd6915edb281a21e49f1be2aac3a626 Mon Sep 17 00:00:00 2001
From: Pascal Jungblut 
Date: Thu, 30 May 2024 14:28:50 +0200
Subject: [PATCH] Add option to ignore anonymous namespaces in
 avoid-non-const-global-variables

---
 .../AvoidNonConstGlobalVariablesCheck.cpp | 19 +---
 .../AvoidNonConstGlobalVariablesCheck.h   |  7 --
 clang-tools-extra/docs/ReleaseNotes.rst   |  5 +
 .../avoid-non-const-global-variables.rst  |  8 +++
 .../avoid-non-const-global-variables.cpp  | 22 ++-
 5 files changed, 51 insertions(+), 10 deletions(-)

diff --git 
a/clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.cpp
 
b/clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.cpp
index ee17b0e014288..a97ec9fe3fe3d 100644
--- 
a/clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.cpp
+++ 
b/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("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
diff --git 
a/clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.h
 
b/clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.h
index e816ca9b47d41..a912763489db9 100644
--- 
a/clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.h
+++ 
b/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
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst 
b/clang-tools-extra/docs/ReleaseNotes.rst
index 4f674d1a4d556..4db476718f775 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -250,6 +250,11 @@ Changes in existing checks
   ` check to also handle
   calls to ``std::forward``.
 
+- Improved :doc:`cppcoreguidelines-avoid-non-const-global-variables
+  ` check
+  with a new option `AllowInternalLinkage` to disable the warning for variables
+  with internal linkage.
+
 - Improved :doc:`cppcoreguidelines-missing-std-forward
   ` check by no longer
   giving false positives for deleted functions, by

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

2024-05-31 Thread Pascal Jungblut via cfe-commits 


@@ -1,29 +1,39 @@
-// RUN: %check_clang_tidy %s 
cppcoreguidelines-avoid-non-const-global-variables %t
+// RUN: %check_clang_tidy %s -check-suffix=DEFAULT 
cppcoreguidelines-avoid-non-const-global-variables %t
+// RUN: %check_clang_tidy %s -check-suffix=NAMESPACE 
cppcoreguidelines-avoid-non-const-global-variables %t -- \

pascalj wrote:

Thanks for this hint, it made the checks much easier. It now tests only for the 
cases where the configuration differs.

https://github.com/llvm/llvm-project/pull/93827
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2024-05-31 Thread Pascal Jungblut via cfe-commits 

pascalj wrote:

Thanks for your feedback!

> * what about "static" non const global variables

Good point, forgot about these. If both are allowed, it is more about the 
internal linkage than it is about the namespace. I renamed the option to 
`AllowInternalLinkage` and permit variables in anonymous namespaces and static 
global variables, since they're equivalent (AFAICT). 

> I'm fine for having an options that control behavior of the check. But please 
> note that there are other aspects of "I.2: Avoid non-const global variables" 
> rule that anonymous namespace or static does not solve. Like race-conditions 
> in multithread env. Or even to force developer not to use global objects to 
> pass data between functions.
> 
> Even that I see many times that warnings from this check are being nolinted, 
> I still think that it's doing good job.

I completely agree with both you and @carlosgalvezp: this is neither an option 
that should be enabled by default, nor should people prefer it over nolinting 
without some thought. However, I see not much harm in giving someone the choice 
to do so. In the end, all of clang-tidy's checks are optional to some degree.

Having said that: if the maintainers prefer not to have this option, this is 
also fine with me.

https://github.com/llvm/llvm-project/pull/93827
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits