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:
> > 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.
================
Comment at:
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.cpp:123-124
+ "member variable %0 provides global access to non-const type, "
+ "consider "
+ "making the referenced data const")
+ << MemberReference; // FIXME: Add fix-it hint to MemberReference
----------------
vingeldal wrote:
> aaron.ballman wrote:
> > Can re-flow this string literal.
> Sorry, I don't understand what you mean by re-flow.
Sorry for not being clear -- the string literal spans three lines when it only
needs to span two by re-writing the literal. e.g.,
```
"member variable %0 provides global access to non-const type, "
"consider making the pointed-to data const"
```
================
Comment at: clang-tools-extra/docs/clang-tidy/checks/list.rst:198
cppcoreguidelines-avoid-magic-numbers (redirects to
readability-magic-numbers) <cppcoreguidelines-avoid-magic-numbers>
+ cppcoreguidelines-avoid-non-const-global-variables
cppcoreguidelines-c-copy-assignment-signature (redirects to
misc-unconventional-assign-operator)
<cppcoreguidelines-c-copy-assignment-signature>
----------------
vingeldal wrote:
> sylvestre.ledru wrote:
> > list.rst changed, you should update this!
> > Thanks
> >
> I'm a bit concerned with this previous change of of list.rst.
>
> Now that I need to add a check to this file, I don't know what severity to
> give it. I can't find any definition of severity levels and I really want to
> avoid getting into a long discussion, or having different reviewers not
> agreeing on my patch, concerning what severity we should give this check.
> Is there any way I can find some kind of guideline on how to set the severity
> to avoid an opinionated discussion about severity level?
I'd like to follow that discussion so that I can be consistent with my review
advice, too.
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
