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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.cpp:29
+      unless(anyOf(
+          isConstexpr(), hasDeclContext(namespaceDecl(isAnonymous())),
+          hasType(isConstQualified()),
----------------
vingeldal wrote:
> aaron.ballman wrote:
> > vingeldal wrote:
> > > 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.
> > > This should probably be clarified in the actual guideline.
> > 
> > This sort of thing comes up with coding guidelines from time to time and the way we usually handle it is to ask the guideline authors for guidance. If they come back with an answer, then we go that route (while the authors fix the text) and if they don't come back with an answer, we muddle our way through. Would you mind filing an issue with the C++ Core Guideline folks to see if they can weigh in on the topic? If they don't respond in a timely fashion, I think it would make more sense to err on the side of caution and diagnose declarations within anonymous namespaces. This matches the current text from the core guideline, and we can always relax the rule if we find it causes a lot of heartache in the wild. (If you have data about how often this particular aspect of the check triggers on large real-world code bases, that could help us make a decision too.)
> I sent a pull request to the guide lines, suggesting a clarification. If that is addressed in the near future I guess we can go on what they say about the pull request. If it takes to long I'll just modify the code to warn in anonymous namespace as well.
> https://github.com/isocpp/CppCoreGuidelines/pull/1553
Thank you, I think that approach makes sense.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70265/new/

https://reviews.llvm.org/D70265