[PATCH] D59523: Thread Safety: also look at ObjC methods

Aaron Puchert via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Wed Mar 20 14:03:39 PDT 2019


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





More information about the cfe-commits mailing list