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

[Caslyn Tonelli via Phabricator via cfe-commits]

Caslyn added a comment.

Hi Piotr - I'm sorry for the delay in getting back to you. Thank you again for your review comments. I did my best trying to get the right combination of matchers that limit the candidates and allow the exceptions that we want. I wasn't successful in figuring out a way to exempt static references to non-trivial destructor classes that don't have the lifetime extension (see comment).



================
Comment at: clang-tools-extra/clang-tidy/fuchsia/GlobalVariablesCheck.cpp:35
+  Finder->addMatcher(
+      varDecl(is_global_or_static_candidate,
+              unless(hasType(cxxRecordDecl(hasAnyName(IgnoreVars)))))
----------------
PiotrZSL wrote:
> only classes can be non trivial to destroy, so we should exclude on this level all types that are not CXXRecordDecl.
In the latest revision I narrowed the matching candidates to: 

```
 anyOf(hasType(hasCanonicalType(references(qualType()))),
                hasType(arrayType()),
                hasType(cxxRecordDecl(hasNonTrivialDestructor()))),
```

I found I needed to capture arrays and references in the tests and included those in the limited candidates.

However, after a lot of testing and experimenting with `materializeTemporaryExpr` and your suggestions, I still couldn't figure out a way to allow references without lifetime extensions. For ex, this test gives a false positive with the latest patch:

```
  // Static references with function scope are allowed if they don't have
  // lifetime-extension.
  static const NonTriviallyDestructibleClass &ConstRefFuncNtdc = *new NonTriviallyDestructibleClass;
```

I've been starting to question if this is a general enough use case to include as an exception. Do you think it would be a mistake if this check does not allow static references to non-trivial destructors, regardless of a lifetime extension?  


================
Comment at: clang-tools-extra/clang-tidy/fuchsia/GlobalVariablesCheck.cpp:36
+      varDecl(is_global_or_static_candidate,
+              unless(hasType(cxxRecordDecl(hasAnyName(IgnoreVars)))))
+          .bind("global_var"),
----------------
PiotrZSL wrote:
> PiotrZSL wrote:
> > could be better, like in other checks.
> 
I went ahead and combined the two suggestions around the `IgnoreVars` matching .


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/fuchsia/global-variables.cpp:103
+ntdc_ref typedef_ref_ntdc =
+    *new NonTriviallyDestructibleClass;
+
----------------
PiotrZSL wrote:
> that will act just like alias
> ``NonTriviallyDestructibleClass XYZ;
> typedef_ref_ntdc  = XYZ;``
> this ``new`` here is confusing... both examples should be made simpler.
I got rid of these scenarios and tested typedef to a reference per your suggestion in the latest patch - hopefully I captured it correctly:

> typedef for const reference of non trivial type that is used to exend lfietime of variable (calling function that returns object with non trivial destructor).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152953