aaronpuchert added a reviewer: aaronpuchert.
aaronpuchert added inline comments.
================
Comment at: lib/Analysis/ThreadSafetyCommon.cpp:282-285
+ const auto *Canonical = Ctx->AttrDecl->getCanonicalDecl();
+ if (isa<FunctionDecl>(D)
+ ? (cast<FunctionDecl>(D)->getCanonicalDecl() == Canonical)
+ : (cast<ObjCMethodDecl>(D)->getCanonicalDecl() == Canonical)) {
----------------
Unrelated to your change, but I'm wondering if this is right. `Ctx->AttrDecl`
is a `NamedDecl`, on which `getCanonicalDecl()` just returns `*this` (so we
could in fact omit it here without functional change), while on `FunctionDecl`
and `ObjCMethodDecl` it does something non-trivial instead.
I guess I need to look into this a bit, don't let this bother you. But I think
we might actually have to do the cast on both sides of the equation to get the
same kind of canonical declaration. Or we make sure that `Ctx->AttrDecl` is
already canonical.
================
Comment at: test/SemaObjCXX/no-crash-thread-safety-analysis.mm:1
+// RUN: %clang_cc1 -fsyntax-only -verify -Wthread-safety -Wno-objc-root-class
%s
+
----------------
Test is fine for me, but I would like if you could integrate it with the
existing test/SemaObjCXX/warn-thread-safety-analysis.mm. The thread safety
analysis requires a bit of setup, that's why we tend to keep the tests in one
file. I'll admit that the C++ tests have grown quite large, but for ObjC++ it's
still very manageable.
================
Comment at: test/SemaObjCXX/no-crash-thread-safety-analysis.mm:10
+
+template <class T> struct __attribute__((scoped_lockable)) Locker {
+ T &_l;
----------------
Can we hard-code `T` = `MyLock`? We can still change it if we need it more
general later.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D59523/new/
https://reviews.llvm.org/D59523
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
