dcoughlin added a comment.
Thanks!
Another round of comments inline. With those addressed it looks good to me --
but you should wait on Artem's go-ahead before committing.
================
Comment at: lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp:121
+ QualType Q = C.getVariable()->getType();
+ if (Q->isObjCObjectPointerType() &&
+ Q->getPointeeType()
----------------
I think here you will need to check whether Q is a TypedefType and compare the
IdentifierInfo for its TypedefNameDecl to 'dispatch_semaphore_t' instead.
The reason is that `NSObject<OS_dispatch_semaphore>` is an implementation
detail and in fact `dispatch_semaphore_t` is typedef'd to another type
depending on whether the file is Objective-C(++) or not.
There is an example of the idiom we use to do IdentifierInfo comparisons in
ObjCDeallocChecker. We cache the identifier in the checker class. This lets us
get constant-time comparisons on identifiers (since they are interned strings).
Also: It would be good to add a comment explaining why we care whether a
semaphore was captured.
================
Comment at: lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp:316
- for (unsigned i = 0, e = cb.V.size(); i != e; ++i) {
+ for (const auto &P : Cb.V) {
// Generate a report for this bug.
----------------
We generally prefer to avoid combining style cleanups like these with
functional changes except in the code that is required to be updated for the
functional change. This makes it easier for future maintainer to use the commit
history to understand what a commit did and why.
================
Comment at: test/Analysis/stack-async-leak.m:1
+// RUN: %clang_analyze_cc1 -triple x86_64-apple-darwin10
-analyzer-checker=core -fblocks -fobjc-arc -verify %s
+
----------------
alexshap wrote:
> dcoughlin wrote:
> > Can you add an additional run line testing the expected behavior when ARC
> > is not enabled? You can pass a -DARC_ENABLED=1 or similar to distinguish
> > between the two (for example, to test to make sure that a diagnostic is
> > emitted under MRR but not ARC).
> i've tried this, it somehow gets more complicated to read / change the tests
> - decided to create a separate file.
That's fine!
================
Comment at: test/Analysis/stack-capture-leak-arc.mm:144
+ block();
+ return block;
+}
----------------
Can you add a `// no-warning` comment to this line? We use that to indicate
that we explicitly don't expect a warning there.
Repository:
rL LLVM
https://reviews.llvm.org/D39438
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
