[clang-tools-extra] [clang-tidy] Option to ignore anonymous namespaces in avoid-non-const-global-variables (PR #93827)
github-actions[bot] wrote: @pascalj Congratulations on having your first Pull Request (PR) merged into the LLVM Project! Your changes will be combined with recent changes from other authors, then tested by our [build bots](https://lab.llvm.org/buildbot/). If there is a problem with a build, you may receive a report in an email or a comment on this PR. Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues. How to do this, and the rest of the post-merge process, is covered in detail [here](https://llvm.org/docs/MyFirstTypoFix.html#myfirsttypofix-issues-after-landing-your-pr). If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of [LLVM development](https://llvm.org/docs/DeveloperPolicy.html#patch-reversion-policy). You can fix your changes and open a new PR to merge them again. If you don't get any reports, no action is required from you. Your changes are working as expected, well done! 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)
https://github.com/PiotrZSL closed 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)
https://github.com/PiotrZSL 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 ) {
-
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 ) {
+ 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 ) override;
+ void storeOptions(ClangTidyOptions::OptionMap ) 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 fixing false negatives
[clang-tools-extra] [clang-tidy] Option to ignore anonymous namespaces in avoid-non-const-global-variables (PR #93827)
https://github.com/HerrCai0907 approved this pull request. 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)
https://github.com/5chmidti approved this pull request. While I'm leaning more towards > NOLINT in the code can grab the attention of a future developer and prompt a > refactoring to avoid the pattern. , it's an off-by-default option that provides a choice. LGTM 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)
5chmidti wrote:
FYI: You could remove the explicit check-nots here:
```c++
// RUN: %check_clang_tidy %s -check-suffixes=,INTERNAL-LINKAGE
cppcoreguidelines-avoid-non-const-global-variables %t
// RUN: %check_clang_tidy %s cppcoreguidelines-avoid-non-const-global-variables
%t -- \
// RUN: -config="{CheckOptions:
{cppcoreguidelines-avoid-non-const-global-variables.AllowInternalLinkage :
'true'}}"
namespace {
int nonConstAnonymousNamespaceInt = 0;
// CHECK-MESSAGES-INTERNAL-LINKAGE: :[[@LINE-1]]:5: warning: variable
'nonConstAnonymousNamespaceInt' is non-const and globally accessible, consider
making it const [cppcoreguidelines-avoid-non-const-global-variables]
} // namespace
```
(There are also two check-nots where the line is off by one)
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)
https://github.com/5chmidti edited 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)
@@ -7,21 +7,30 @@
//===--===//
#include "AvoidNonConstGlobalVariablesCheck.h"
-#include "clang/AST/ASTContext.h"
#include "clang/ASTMatchers/ASTMatchFinder.h"
#include "clang/ASTMatchers/ASTMatchers.h"
using namespace clang::ast_matchers;
namespace clang::tidy::cppcoreguidelines {
+AvoidNonConstGlobalVariablesCheck::AvoidNonConstGlobalVariablesCheck(
+StringRef Name, ClangTidyContext *Context)
+: ClangTidyCheck(Name, Context),
+ AllowInternalLinkage(Options.get("AllowInternalLinkage", false)) {}
SimplyDanny wrote:
Option name could be a constant to avoid duplication.
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)
https://github.com/SimplyDanny approved this pull request. 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)
https://github.com/SimplyDanny edited 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)
https://github.com/PiotrZSL approved this pull request. LGTM 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)
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
[clang-tools-extra] [clang-tidy] Option to ignore anonymous namespaces in avoid-non-const-global-variables (PR #93827)
@@ -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)
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 ) {
-
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 ) {
+ 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 ) override;
+ void storeOptions(ClangTidyOptions::OptionMap ) 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 fixing false negatives
[clang-tools-extra] [clang-tidy] Option to ignore anonymous namespaces in avoid-non-const-global-variables (PR #93827)
carlosgalvezp wrote: I have a bit the feeling that users should use NOLINT* here instead of having a global option that is not seen in the code. Anonymous namespaces do not solve many of the remaining issues. A NOLINT in the code can grab the attention of a future developer and prompt a refactoring to avoid the pattern. I don't have a strong opinion so if this is really wanted I won't object. 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)
@@ -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`.
PiotrZSL wrote:
I dont think that this example is needed. Check option already says a lot.
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)
@@ -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. PiotrZSL wrote: wrap on 80 column 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)
@@ -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 -- \ PiotrZSL wrote: use -check-suffixes=,NAMESPACE instead of duplicating all CHECK-MESSGAES or adding DEFAULT 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)
@@ -16,9 +15,12 @@ using namespace clang::ast_matchers;
namespace clang::tidy::cppcoreguidelines {
void AvoidNonConstGlobalVariablesCheck::registerMatchers(MatchFinder *Finder) {
+ auto NamespaceMatcher = Options.get("AllowAnonymousNamespace", false)
PiotrZSL wrote:
Options.get should be called from constructor and saved into class member.
storeOption is missing or not updated.
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)
https://github.com/PiotrZSL edited 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)
https://github.com/PiotrZSL requested changes to this pull request. - storeOptions is missing - release notes need to be updated about added new option - tests need some love (reduce amount of changes) - what about "static" non const global variables 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. 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)
@@ -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. EugeneZelenko wrote: Default value is usually stated in dedicated sentence at the end of paragraph. 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)
llvmbot wrote:
@llvm/pr-subscribers-clang-tidy
Author: Pascal Jungblut (pascalj)
Changes
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.
---
Patch is 36.15 KiB, truncated to 20.00 KiB below, full version:
https://github.com/llvm/llvm-project/pull/93827.diff
3 Files Affected:
- (modified)
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.cpp
(+4-2)
- (modified)
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/avoid-non-const-global-variables.rst
(+22)
- (modified)
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-non-const-global-variables.cpp
(+92-45)
``diff
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:
[clang-tools-extra] [clang-tidy] Option to ignore anonymous namespaces in avoid-non-const-global-variables (PR #93827)
github-actions[bot] wrote: Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using `@` followed by their GitHub username. If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the [LLVM GitHub User Guide](https://llvm.org/docs/GitHub.html). You can also ask questions in a comment on this PR, on the [LLVM Discord](https://discord.com/invite/xS7Z362) or on the [forums](https://discourse.llvm.org/). 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)
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
