[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 "
----------------
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.


================
Comment at: clang-tools-extra/docs/ReleaseNotes.rst:107
+
+  Finds non-const global variables as described in check I.2 of cpp core
+  guidelines.
----------------
JonasToth wrote:
> The official name is "C++ Core Guidelines", i think we should stick to that.
> And please ellide the new empty line above.
I'll remove the empty line,  I must ask about this formatting though.
I see multiple examples both with and without the empty line, so I just tried to be consistent with what most people seem to do.
What's the rule for knowing when to use one and when not to?


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-avoid-non-const-global-variables.cpp:46
+  int nonConstMemberVariable = 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: member variable 'nonConstMemberVariable' is globally accessible and non-const, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
+  const int constMemberVariable = 0;
----------------
JonasToth wrote:
> diagnosing those might be undesired. maybe having an option to enable/disable this would be nice?
> We should try to allow reducing the noise level of clang-tidy.
I can see your point and I agree that an option would be a good idea.
It isn't obvious that members variable could be considered global variables.
I'll make the current behavior the default though, since I got the impression
that in most  aligned with the intent of the guideline.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70265