aaronpuchert added inline comments.
================
Comment at: lib/Analysis/ThreadSafetyCommon.cpp:283-285
+ if (isa<FunctionDecl>(D)
+ ? (cast<FunctionDecl>(D)->getCanonicalDecl() == Canonical)
+ : (cast<ObjCMethodDecl>(D)->getCanonicalDecl() == Canonical)) {
----------------
aaron.ballman wrote:
> Also somewhat orthogonal to your changes, but... ugh, the lack of any common
> base between `FunctionDecl` and `ObjcMethodDecl` strikes again. I sort of
> wish we would introduce a CallableDecl base class that these two (and
> BlockDecl) could all inherit from to take care of this sort of ugly hack.
Seems I was wrong, because `getCanonicalDecl()` is actually virtual. I was
confused by the different return type, apparently an overriding function
doesn't need to have the same return type. So no problem here.
This doesn't make our case easier: both `FunctionDecl` and `ObjCMethodDecl`
inherit from both `Decl` and `DeclContext`, but independently. So we can't
directly cast from `DeclContext` to `Decl`, we need to know which leaf we are.
================
Comment at: test/SemaObjCXX/no-crash-thread-safety-analysis.mm:1
+// RUN: %clang_cc1 -fsyntax-only -verify -Wthread-safety -Wno-objc-root-class
%s
+
----------------
jfb wrote:
> aaronpuchert wrote:
> > 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.
> Sure thing! I created a header that's shared and simplified this repro a bit.
> I validated that the shared code was crashing before and this fixes the crash.
I would still like all ObjCXX tests for -Wthread-safety in one file, because
that's what we have for the other languages as well. (more or less)
It's somewhat easier to see which aspects are tested then and which are not.
Right now we only have tests for reported crashes, but maybe someday someone
finds the time to test a bit more exhaustively.
But perhaps @aaron.ballman sees this another way, in that case follow his
opinion.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D59523/new/
https://reviews.llvm.org/D59523
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
