[PATCH] D152953: [clang-tidy] Introduce fuchsia-global-variables check

[Piotr Zegar via Phabricator via cfe-commits]

PiotrZSL requested changes to this revision.
PiotrZSL added a comment.
This revision now requires changes to proceed.

This check overlap with fuchsia-statically-constructed-objects.
In fact only new warnings are in lines 118, 111, 99.

Provided link is dead.
Fuchsia codding standard does not forbid global objects with non-trivial destructors: https://fuchsia.dev/fuchsia-src/development/languages/c-cpp/cxx?hl=en

Looks like what you trying to implement is an google style rule: "https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables" -> "Objects with static storage duration are forbidden unless they are trivially destructible. "
Therefor this check should be in google namespace, not fuchsia.
Please also look into: https://github.com/llvm/llvm-project/issues/62334

Personally I would see this as some generic misc-non-trivial-global-variable that could handle both initialization & destruction, and simply some standards could alias it.



================
Comment at: clang-tools-extra/clang-tidy/fuchsia/GlobalVariablesCheck.cpp:30
+            // Exception: references without lifetime-extension.
+            unless(allOf(hasType(references(qualType())),
+                         unless(hasInitializer(exprWithCleanups(
----------------
may not work with typedefs to reference


================
Comment at: clang-tools-extra/clang-tidy/fuchsia/GlobalVariablesCheck.cpp:31-32
+            unless(allOf(hasType(references(qualType())),
+                         unless(hasInitializer(exprWithCleanups(
+                             has(materializeTemporaryExpr())))))));
+
----------------
there can be some implicitCasts here, and it wont match, also CXXBindTemporaryExpr can show up, this isn't a good way...


================
Comment at: clang-tools-extra/clang-tidy/fuchsia/GlobalVariablesCheck.cpp:38
+          varDecl(is_global_or_static_candidate,
+                  unless(hasType(cxxRecordDecl(hasAnyName(IgnoreVars)))),
+                  // Special handling for std::array is treated below
----------------
what about typedefs ?


================
Comment at: clang-tools-extra/clang-tidy/fuchsia/GlobalVariablesCheck.cpp:46
+      ast_matchers::traverse(
+          TK_AsIs, varDecl(hasType(cxxRecordDecl(hasName("::std::array"))),
+                           is_global_or_static_candidate)
----------------
TK_AsIs is default.


================
Comment at: clang-tools-extra/clang-tidy/fuchsia/GlobalVariablesCheck.cpp:86-87
+  diag(decl->getLocation(),
+       "static and global variables are forbidden unless they are trivially "
+       "destructible");
+}
----------------
do not use unless, because other checks make set other rules, and we could run into conflict between them


================
Comment at: clang-tools-extra/clang-tidy/fuchsia/GlobalVariablesCheck.cpp:96
+                 Result.Nodes.getNodeAs<VarDecl>("global_var_std_array")) {
+    // In the case of std::array, check the type of the first template argument.
+    // Zero-size arrays are always allowed.
----------------
if you put non trivial type into std::array, then std::array instance will become non-trivial itself, no need to look into arguments.


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/fuchsia/global-variables.rst:28
+    NonPODClass(int Var) : Var(Var) {}
+    ~NonPODClass() {}
+    int Var;
----------------
wrong example, `using = default` will make it trivial, put something into destructor.


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/fuchsia/global-variables.rst:39
+`thread_local` variables in functions are permitted without restrictions.
+
+Options
----------------
would be nice to point out to a rule, that it implements.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152953