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



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to