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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
