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