[clang-tidy] Option to ignore anonymous namespaces in avoid-non-const-global-variables #93827
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be 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 If you have received no comments on your PR for a week, you can request a review If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
|
@llvm/pr-subscribers-clang-tools-extra @llvm/pr-subscribers-clang-tidy Author: Pascal Jungblut (pascalj) ChangesAdd 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 isocpp/CppCoreGuidelines#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:
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 and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
int &nonConstIntReference = nonConstInt;
-// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: variable 'nonConstIntReference' provides global access to a non-const object; consider making the referenced data 'const' [cppcoreguidelines-avoid-non-const-global-variables]
+// CHECK-MESSAGES-DEFAULT: [[@LINE-1]]:6: warning: variable 'nonConstIntReference' provides global access to a non-const object; consider making the referenced data 'const' [cppcoreguidelines-avoid-non-const-global-variables]
+// CHECK-MESSAGES-NAMESPACE: [[@LINE-2]]:6: warning: variable 'nonConstIntReference' provides global access to a non-const object; consider making the referenced data 'const' [cppcoreguidelines-avoid-non-const-global-variables]
int *pointerToNonConstInt = &nonConstInt;
-// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: variable 'pointerToNonConstInt' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
-// CHECK-MESSAGES: :[[@LINE-2]]:6: warning: variable 'pointerToNonConstInt' provides global access to a non-const object; consider making the pointed-to data 'const' [cppcoreguidelines-avoid-non-const-global-variables]
+// CHECK-MESSAGES-DEFAULT: [[@LINE-1]]:6: warning: variable 'pointerToNonConstInt' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
+// CHECK-MESSAGES-NAMESPACE: [[@LINE-2]]:6: warning: variable 'pointerToNonConstInt' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
+// CHECK-MESSAGES-DEFAULT: [[@LINE-3]]:6: warning: variable 'pointerToNonConstInt' provides global access to a non-const object; consider making the pointed-to data 'const' [cppcoreguidelines-avoid-non-const-global-variables]
+// CHECK-MESSAGES-NAMESPACE: [[@LINE-4]]:6: warning: variable 'pointerToNonConstInt' provides global access to a non-const object; consider making the pointed-to data 'const' [cppcoreguidelines-avoid-non-const-global-variables]
int *const constPointerToNonConstInt = &nonConstInt;
-// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: variable 'constPointerToNonConstInt' provides global access to a non-const object; consider making the pointed-to data 'const' [cppcoreguidelines-avoid-non-const-global-variables]
+// CHECK-MESSAGES-DEFAULT: [[@LINE-1]]:12: warning: variable 'constPointerToNonConstInt' provides global access to a non-const object; consider making the pointed-to data 'const' [cppcoreguidelines-avoid-non-const-global-variables]
+// CHECK-MESSAGES-NAMESPACE: [[@LINE-2]]:12: warning: variable 'constPointerToNonConstInt' provides global access to a non-const object; consider making the pointed-to data 'const' [cppcoreguidelines-avoid-non-const-global-variables]
namespace namespace_name {
int nonConstNamespaceInt = 0;
-// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: variable 'nonConstNamespaceInt' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
+// CHECK-MESSAGES-DEFAULT: [[@LINE-1]]:5: warning: variable 'nonConstNamespaceInt' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
+// CHECK-MESSAGES-NAMESPACE: [[@LINE-2]]:5: warning: variable 'nonConstNamespaceInt' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
const int constNamespaceInt = 0;
} // namespace namespace_name
@@ -23,7 +32,8 @@ const int constNamespaceInt = 0;
const int constInt = 0;
const int *pointerToConstInt = &constInt;
-// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: variable 'pointerToConstInt' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
+// CHECK-MESSAGES-DEFAULT: [[@LINE-1]]:12: warning: variable 'pointerToConstInt' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
+// CHECK-MESSAGES-NAMESPACE: [[@LINE-2]]:12: warning: variable 'pointerToConstInt' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
const int *const constPointerToConstInt = &constInt;
@@ -38,7 +48,8 @@ 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-NAMESPACE-NOT: :[[@LINE-2]]:5: warning: variable 'nonConstAnonymousNamespaceInt' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
} // namespace
class DummyClass {
@@ -52,28 +63,37 @@ class DummyClass {
};
DummyClass nonConstClassInstance;
-// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: variable 'nonConstClassInstance' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
+// CHECK-MESSAGES-DEFAULT: [[@LINE-1]]:12: warning: variable 'nonConstClassInstance' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
+// CHECK-MESSAGES-NAMESPACE: [[@LINE-2]]:12: warning: variable 'nonConstClassInstance' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
DummyClass *pointerToNonConstDummyClass = &nonConstClassInstance;
-// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: variable 'pointerToNonConstDummyClass' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
-// CHECK-MESSAGES: :[[@LINE-2]]:13: warning: variable 'pointerToNonConstDummyClass' provides global access to a non-const object; consider making the pointed-to data 'const' [cppcoreguidelines-avoid-non-const-global-variables]
+// CHECK-MESSAGES-DEFAULT: [[@LINE-1]]:13: warning: variable 'pointerToNonConstDummyClass' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
+// CHECK-MESSAGES-NAMESPACE: [[@LINE-2]]:13: warning: variable 'pointerToNonConstDummyClass' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
+// CHECK-MESSAGES-DEFAULT: [[@LINE-3]]:13: warning: variable 'pointerToNonConstDummyClass' provides global access to a non-const object; consider making the pointed-to data 'const' [cppcoreguidelines-avoid-non-const-global-variables]
+// CHECK-MESSAGES-NAMESPACE: [[@LINE-4]]:13: warning: variable 'pointerToNonConstDummyClass' provides global access to a non-const object; consider making the pointed-to data 'const' [cppcoreguidelines-avoid-non-const-global-variables]
DummyClass &referenceToNonConstDummyClass = nonConstClassInstance;
-// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: variable 'referenceToNonConstDummyClass' provides global access to a non-const object; consider making the referenced data 'const' [cppcoreguidelines-avoid-non-const-global-variables]
+// CHECK-MESSAGES-DEFAULT: [[@LINE-1]]:13: warning: variable 'referenceToNonConstDummyClass' provides global access to a non-const object; consider making the referenced data 'const' [cppcoreguidelines-avoid-non-const-global-variables]
+// CHECK-MESSAGES-NAMESPACE: [[@LINE-2]]:13: warning: variable 'referenceToNonConstDummyClass' provides global access to a non-const object; consider making the referenced data 'const' [cppcoreguidelines-avoid-non-const-global-variables]
int *nonConstPointerToMember = &nonConstClassInstance.nonConstPublicMemberVariable;
-// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: variable 'nonConstPointerToMember' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
-// CHECK-MESSAGES: :[[@LINE-2]]:6: warning: variable 'nonConstPointerToMember' provides global access to a non-const object; consider making the pointed-to data 'const' [cppcoreguidelines-avoid-non-const-global-variables]
+// CHECK-MESSAGES-DEFAULT: [[@LINE-1]]:6: warning: variable 'nonConstPointerToMember' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
+// CHECK-MESSAGES-NAMESPACE: [[@LINE-2]]:6: warning: variable 'nonConstPointerToMember' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
+// CHECK-MESSAGES-DEFAULT: [[@LINE-3]]:6: warning: variable 'nonConstPointerToMember' provides global access to a non-const object; consider making the pointed-to data 'const' [cppcoreguidelines-avoid-non-const-global-variables]
+// CHECK-MESSAGES-NAMESPACE: [[@LINE-4]]:6: warning: variable 'nonConstPointerToMember' provides global access to a non-const object; consider making the pointed-to data 'const' [cppcoreguidelines-avoid-non-const-global-variables]
int *const constPointerToNonConstMember = &nonConstClassInstance.nonConstPublicMemberVariable;
-// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: variable 'constPointerToNonConstMember' provides global access to a non-const object; consider making the pointed-to data 'const' [cppcoreguidelines-avoid-non-const-global-variables]
+// CHECK-MESSAGES-DEFAULT: [[@LINE-1]]:12: warning: variable 'constPointerToNonConstMember' provides global access to a non-const object; consider making the pointed-to data 'const' [cppcoreguidelines-avoid-non-const-global-variables]
+// CHECK-MESSAGES-NAMESPACE: [[@LINE-2]]:12: warning: variable 'constPointerToNonConstMember' provides global access to a non-const object; consider making the pointed-to data 'const' [cppcoreguidelines-avoid-non-const-global-variables]
const DummyClass constClassInstance;
DummyClass *const constPointerToNonConstDummyClass = &nonConstClassInstance;
-// CHECK-MESSAGES: :[[@LINE-1]]:19: warning: variable 'constPointerToNonConstDummyClass' provides global access to a non-const object; consider making the pointed-to data 'const' [cppcoreguidelines-avoid-non-const-global-variables]
+// CHECK-MESSAGES-DEFAULT: [[@LINE-1]]:19: warning: variable 'constPointerToNonConstDummyClass' provides global access to a non-const object; consider making the pointed-to data 'const' [cppcoreguidelines-avoid-non-const-global-variables]
+// CHECK-MESSAGES-NAMESPACE: [[@LINE-2]]:19: warning: variable 'constPointerToNonConstDummyClass' provides global access to a non-const object; consider making the pointed-to data 'const' [cppcoreguidelines-avoid-non-const-global-variables]
const DummyClass *nonConstPointerToConstDummyClass = &constClassInstance;
-// CHECK-MESSAGES: :[[@LINE-1]]:19: warning: variable 'nonConstPointerToConstDummyClass' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
+// CHECK-MESSAGES-DEFAULT: [[@LINE-1]]:19: warning: variable 'nonConstPointerToConstDummyClass' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
+// CHECK-MESSAGES-NAMESPACE: [[@LINE-2]]:19: warning: variable 'nonConstPointerToConstDummyClass' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
const DummyClass *const constPointerToConstDummyClass = &constClassInstance;
@@ -83,7 +103,8 @@ const DummyClass &constReferenceToDummyClass = constClassInstance;
namespace namespace_name {
DummyClass nonConstNamespaceClassInstance;
-// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: variable 'nonConstNamespaceClassInstance' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
+// CHECK-MESSAGES-DEFAULT: [[@LINE-1]]:12: warning: variable 'nonConstNamespaceClassInstance' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
+// CHECK-MESSAGES-NAMESPACE: [[@LINE-2]]:12: warning: variable 'nonConstNamespaceClassInstance' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
const DummyClass constDummyClassInstance;
} // namespace namespace_name
@@ -95,22 +116,28 @@ enum DummyEnum {
};
DummyEnum nonConstDummyEnumInstance = DummyEnum::first;
-// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: variable 'nonConstDummyEnumInstance' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
+// CHECK-MESSAGES-DEFAULT: [[@LINE-1]]:11: warning: variable 'nonConstDummyEnumInstance' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
+// CHECK-MESSAGES-NAMESPACE: [[@LINE-2]]:11: warning: variable 'nonConstDummyEnumInstance' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
DummyEnum *pointerToNonConstDummyEnum = &nonConstDummyEnumInstance;
-// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: variable 'pointerToNonConstDummyEnum' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
-// CHECK-MESSAGES: :[[@LINE-2]]:12: warning: variable 'pointerToNonConstDummyEnum' provides global access to a non-const object; consider making the pointed-to data 'const' [cppcoreguidelines-avoid-non-const-global-variables]
+// CHECK-MESSAGES-DEFAULT: [[@LINE-1]]:12: warning: variable 'pointerToNonConstDummyEnum' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
+// CHECK-MESSAGES-NAMESPACE: [[@LINE-2]]:12: warning: variable 'pointerToNonConstDummyEnum' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
+// CHECK-MESSAGES-DEFAULT: [[@LINE-3]]:12: warning: variable 'pointerToNonConstDummyEnum' provides global access to a non-const object; consider making the pointed-to data 'const' [cppcoreguidelines-avoid-non-const-global-variables]
+// CHECK-MESSAGES-NAMESPACE: [[@LINE-4]]:12: warning: variable 'pointerToNonConstDummyEnum' provides global access to a non-const object; consider making the pointed-to data 'const' [cppcoreguidelines-avoid-non-const-global-variables]
DummyEnum &referenceToNonConstDummyEnum = nonConstDummyEnumInstance;
-// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: variable 'referenceToNonConstDummyEnum' provides global access to a non-const object; consider making the referenced data 'const' [cppcoreguidelines-avoid-non-const-global-variables]
+// CHECK-MESSAGES-DEFAULT: [[@LINE-1]]:12: warning: variable 'referenceToNonConstDummyEnum' provides global access to a non-const object; consider making the referenced data 'const' [cppcoreguidelines-avoid-non-const-global-variables]
+// CHECK-MESSAGES-NAMESPACE: [[@LINE-2]]:12: warning: variable 'referenceToNonConstDummyEnum' provides global access to a non-const object; consider making the referenced data 'const' [cppcoreguidelines-avoid-non-const-global-variables]
DummyEnum *const constPointerToNonConstDummyEnum = &nonConstDummyEnumInstance;
-// CHECK-MESSAGES: :[[@LINE-1]]:18: warning: variable 'constPointerToNonConstDummyEnum' provides global access to a non-const object; consider making the pointed-to data 'const' [cppcoreguidelines-avoid-non-const-global-variables]
+// CHECK-MESSAGES-DEFAULT: [[@LINE-1]]:18: warning: variable 'constPointerToNonConstDummyEnum' provides global access to a non-const object; consider making the pointed-to data 'const' [cppcoreguidelines-avoid-non-const-global-variables]
+// CHECK-MESSAGES-NAMESPACE: [[@LINE-2]]:18: warning: variable 'constPointerToNonConstDummyEnum' provides global access to a non-const object; consider making the pointed-to data 'const' [cppcoreguidelines-avoid-non-const-global-variables]
const DummyEnum constDummyEnumInstance = DummyEnum::first;
const DummyEnum *nonConstPointerToConstDummyEnum = &constDummyEnumInstance;
-// CHECK-MESSAGES: :[[@LINE-1]]:18: warning: variable 'nonConstPointerToConstDummyEnum' is non-const and globally accessible, consider making it const [c...
[truncated]
|
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/avoid-non-const-global-variables.rst
Outdated
Show resolved
Hide resolved
EugeneZelenko
requested review from
PiotrZSL,
SimplyDanny,
5chmidti and
HerrCai0907
PiotrZSL
requested changes
May 30, 2024
There was a problem hiding this comment.
- 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.
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.cpp
Outdated
Show resolved
Hide resolved
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/avoid-non-const-global-variables.rst
Outdated
Show resolved
Hide resolved
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/avoid-non-const-global-variables.rst
Outdated
Show resolved
Hide resolved
...-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-non-const-global-variables.cpp
Outdated
Show resolved
Hide resolved
|
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. |
|
Thanks for your feedback!
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
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. |
SimplyDanny
approved these changes
Jun 15, 2024
| AvoidNonConstGlobalVariablesCheck::AvoidNonConstGlobalVariablesCheck( | ||
| StringRef Name, ClangTidyContext *Context) | ||
| : ClangTidyCheck(Name, Context), | ||
| AllowInternalLinkage(Options.get("AllowInternalLinkage", false)) {} |
There was a problem hiding this comment.
Option name could be a constant to avoid duplication.
5chmidti
approved these changes
Jun 15, 2024
There was a problem hiding this comment.
FYI: You could remove the explicit check-nots here:
// 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)
HerrCai0907
approved these changes
Jun 15, 2024
|
@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 Please check whether problems have been caused by your change specifically, as How to do this, and the rest of the post-merge process, is covered in detail here. If your change does cause a problem, it may be reverted, or you can revert it yourself. If you don't get any reports, no action is required from you. Your changes are working as expected, well done! |
lravenclaw
pushed a commit
to lravenclaw/llvm-project
that referenced
this pull request
Jul 3, 2024
…-global-variables (llvm#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 isocpp/CppCoreGuidelines#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.
kbluck
pushed a commit
to kbluck/llvm-project
that referenced
this pull request
Jul 6, 2024
…-global-variables (llvm#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 isocpp/CppCoreGuidelines#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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 isocpp/CppCoreGuidelines#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.