aaron.ballman added inline comments.
Herald added a subscriber: whisperity.
================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.cpp:64
+
+ if (Variable) {
+ diag(Variable->getLocation(), "variable %0 is non-const and globally "
----------------
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.
================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.cpp:29
+ unless(anyOf(
+ isConstexpr(), hasDeclContext(namespaceDecl(isAnonymous())),
+ hasType(isConstQualified()),
----------------
Why are you filtering out anonymous namespaces?
================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.cpp:89
+
+ if (Variable) {
+ diag(Variable->getLocation(), "variable %0 is non-const and globally "
----------------
I'd sink the local variables into the if statement:
```
if (const auto *Var = Result.Nodes.getNodeAs<VarDecl>("non-const_variable")) {
...
}
```
(If you don't want to wrap lines, you can make the string literals a bit shorter.)
================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.cpp:106
+ "variable %0 provides global access to non-const type, consider "
+ "making the pointed to data const")
+ << Pointer; // FIXME: Add fix-it hint to Pointer
----------------
pointed to -> pointed-to
================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.cpp:120
+ if (MemberReference) {
+ ;
+ diag(MemberReference->getLocation(),
----------------
Spurious semicolon
================
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
----------------
Can re-flow this string literal.
================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.cpp:130
+ if (MemberPointer) {
+ ;
+ diag(MemberPointer->getLocation(),
----------------
Spurious semicolon.
================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.cpp:133
+ "member variable %0 provides global access to non-const type, "
+ "consider "
+ "making the pointed to data const")
----------------
Can re-flow this string literal.
================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.cpp:134
+ "consider "
+ "making the pointed to data const")
+ << MemberPointer; // FIXME: Add fix-it hint to MemberPointer
----------------
pointed to -> pointed-to
================
Comment at: clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-avoid-non-const-global-variables.rst:9
+By default this check considers public member variables to be global variables,
+there is an option, NoMembers, to turn of checks of member variables.
+https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Ri-global
----------------
Add backticks around things that should be in code font like `NoMembers`.
================
Comment at: clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-avoid-non-const-global-variables.rst:44
+
+Variables: a, c, e_ptr1, e_ptr2, e_const_ptr, e_reference and f, will all
+generate warnings since they are either: a globally accessible variable and
----------------
Backticks here as well.
================
Comment at: clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-avoid-non-const-global-variables.rst:52
+
+.. option:: NoMembers (default = 0)
+
----------------
Rather than `NoMembers`, how about going with `IgnoreDataMembers`?
================
Comment at: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-avoid-non-const-global-variables-NoMembers.cpp:25
+};
\ No newline at end of file
----------------
Please add a newline to the end of the file.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D70265/new/
https://reviews.llvm.org/D70265