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
