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

[Caslyn Tonelli via Phabricator via cfe-commits]

Caslyn added a comment.

Thanks for the review @PiotrZSL, I’m new to this space and appreciate the comments. I have a few questions around some of them and would greatly appreciate any guidance you can give.

The intent of this patch is to upstream a generic version of the google style check (apologies for the private link) for use on Fuchsia and other projects. I’m happy to make this check a misc check that handles initialization and destruction if you wouldn’t mind advising a little bit. To handle the initialization aspect, would you suggest I combine logic from the existing fuchsia-statically-constructed-objects check?

> Please also look into: https://github.com/llvm/llvm-project/issues/62334

I see there’s some potential clean up for the fuchsia module. I can take a stab at that ticket after this patch.



================
Comment at: clang-tools-extra/clang-tidy/fuchsia/GlobalVariablesCheck.cpp:31-32
+            unless(allOf(hasType(references(qualType())),
+                         unless(hasInitializer(exprWithCleanups(
+                             has(materializeTemporaryExpr())))))));
+
----------------
PiotrZSL wrote:
> there can be some implicitCasts here, and it wont match, also CXXBindTemporaryExpr can show up, this isn't a good way...
Is there a better way to implement this intent while handling those corner cases?


================
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
----------------
PiotrZSL wrote:
> what about typedefs ?
There is the typedef scenario tested on L#90 the global-variables.cpp test and I've added an additional test for typedef to references - are there any other test cases I can add to make sure this check handles them correctly?



================
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.
----------------
PiotrZSL wrote:
> if you put non trivial type into std::array, then std::array instance will become non-trivial itself, no need to look into arguments.
Done - removed special handling for `std::array`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152953