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:
> > > 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
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
