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