[PATCH] D70265: [clang-tidy] Add clang tidy check I.2 to cppcoreguidelines

[Kim Viggedal via Phabricator via cfe-commits]

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