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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits