vingeldal marked an inline comment as done.
vingeldal added inline comments.
================
Comment at:
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.cpp:29
+ unless(anyOf(
+ isConstexpr(), hasDeclContext(namespaceDecl(isAnonymous())),
+ hasType(isConstQualified()),
----------------
aaron.ballman wrote:
> vingeldal wrote:
> > aaron.ballman wrote:
> > > Why are you filtering out anonymous namespaces?
> > If it's in an anonymous namespace it's no longer globally accessible, it's
> > only accessible within the file it is declared.
> It is, however, at namespace scope which is what the C++ Core Guideline
> suggests to diagnose. Do you have evidence that this is the interpretation
> the guideline authors were looking for (perhaps they would like to update
> their suggested guidance for diagnosing)?
>
> There are two dependency issues to global variables -- one is surprising
> linkage interactions (where the extern nature of the symbol is an issue
> across module boundaries) and the other is surprising name lookup behavior
> within the TU (where the global nature of the symbol is an issue). e.g.,
> ```
> namespace {
> int ik;
> }
>
> void f() {
> for (int ij = 0; ik < 10; ij++) { // Oops, typo, but still compiles
> }
> }
> ```
> That's why I think the guideline purposefully does not exclude things like
> anonymous namespaces.
I don't have any evidence. To me the guideline still looks a bit draft-like, so
I just tried my best guess as to the intent of the guideline.
In reading the guideline I get the impression that the intent is to avoid
globally accessible data which is mutable,
mainly for two reason:
* It's hard to reason about code where you are dependent upon state which can
be changed from anywhere in the code.
* There is a risk of race conditions with this kind of data.
Keeping the variable in an anonymous namespace seems to deals with the issues
which I interpret to be the focus of this guideline.
Consider that the guideline is part of the interface section. If the variable
is declared in an anonymous namespace there is no risk that a user of some
service interface adds a dependency to that variable, since the variable will
be a hidden part of the provider implementation.
Admittedly the wording doesn't mention an exception for anonymous namespaces,
and the sentence you have referenced seems to suggest that any non-const
variable in namespace scope should be matched.
I'm guessing though, that was intended to clarify that a variable is still
global even if in a (named) namespace, because that would not have been an
obvious interpretation otherwise.
Without the referenced sentence one might interpret only variables outside of
namespace scope as global.
Arguably a variable in namespace scope isn't globally declared, though it is
globally accessible, via the namespace name. I think the global accessibility
is the reason for dragging in variables from namespace scope and if that is the
case then we shouldn't also need to worry about anonymous namespaces.
This should probably be clarified in the actual guideline.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D70265/new/
https://reviews.llvm.org/D70265
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
