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

Caslyn Tonelli via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Jun 28 15:08:09 PDT 2023

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: 


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).

  rG LLVM Github Monorepo



More information about the llvm-commits mailing list