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

[Kim Viggedal via Phabricator via cfe-commits]

vingeldal added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.cpp:64
+
+  if (Variable) {
+    diag(Variable->getLocation(), "variable %0 is non-const and globally "
----------------
aaron.ballman wrote:
> vingeldal wrote:
> > JonasToth wrote:
> > > each of those `if`s should `return` early, or could multiple match?
> > > If only one can match the structure could be changed a bit to
> > > ```
> > > if (const auto* Variable = Result.Nodes.getNodeAs<VarDecl>("non-const_variable")) {
> > >     diag(...);
> > >     return;
> > > }
> > > ```
> > > 
> > > If you change the order of the `if`s in the order of matches you expect to happen most, this should be a bit faster as well, as retrieving the matches introduces some overhead that is not relevant in the current situation.
> > > 
> > > If you keep only one statement within the `if` you should ellide the braces, per coding convention.
> > There could be multiple matches but there could still be some early returns.
> > An example of a multi match would be:
> > 
> > namespace {
> > int i = 0;
> > }
> > int * i_ptr = &i;
> > 
> > There would be two warnings for i_ptr, since it is:
> >  1. A global non-const variable it self and...
> >  2. because it globally exposes the non-const variable i.
> > 
> > I'll add early returns where possible.
> > 
> > ...Now that I think about it I realize I'v missed checking for member variables referencing or pointing to non-const data,
> > I'll add that tigether with some more testing.
> Based on my reading of the C++ core guideline, I think there should be a different two diagnostics. One for the declaration of `i` and one for the declaration of `i_ptr`, because of this from the rule:
> 
> > The rule against global variables applies to namespace scope variables as well.
If the variable i was in a **named** namespace it would be matched by a diagnostic, i_ptr is matched by another diagnostic for pointer providing global access to non-const data (as well as the matcher for global non-const variables)


================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.cpp:29
+      unless(anyOf(
+          isConstexpr(), hasDeclContext(namespaceDecl(isAnonymous())),
+          hasType(isConstQualified()),
----------------
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.


================
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
----------------
aaron.ballman wrote:
> Can re-flow this string literal.
Sorry, I don't understand what you mean by re-flow.


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-avoid-non-const-global-variables.rst:52
+
+.. option:: NoMembers (default = 0)
+
----------------
aaron.ballman wrote:
> Rather than `NoMembers`, how about going with `IgnoreDataMembers`?
Yes, that's much better, thanks.


================
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>
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70265