[PATCH] D60736: [Sema][ObjC] Don't warn about a block implicitly retaining self if the block is marked noescape
This revision was automatically updated to reflect the committed changes.
Closed by commit rL358624: [Sema][ObjC] Dont warn about an implicitly
retained self if the (authored by ahatanak, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D60736?vs=195639=195650#toc
Repository:
rL LLVM
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D60736/new/
https://reviews.llvm.org/D60736
Files:
cfe/trunk/include/clang/AST/DeclBase.h
cfe/trunk/include/clang/Sema/Sema.h
cfe/trunk/lib/Sema/SemaDecl.cpp
cfe/trunk/lib/Sema/SemaDeclObjC.cpp
cfe/trunk/lib/Sema/SemaExpr.cpp
cfe/trunk/test/SemaObjC/warn-implicit-self-in-block.m
cfe/trunk/test/SemaObjCXX/warn-implicit-self-in-block.mm
Index: cfe/trunk/include/clang/AST/DeclBase.h
===
--- cfe/trunk/include/clang/AST/DeclBase.h
+++ cfe/trunk/include/clang/AST/DeclBase.h
@@ -41,6 +41,7 @@
class ASTContext;
class ASTMutationListener;
class Attr;
+class BlockDecl;
class DeclContext;
class ExternalSourceSymbolAttr;
class FunctionDecl;
@@ -1792,6 +1793,20 @@
bool isClosure() const { return getDeclKind() == Decl::Block; }
+ /// Return this DeclContext if it is a BlockDecl. Otherwise, return the
+ /// innermost enclosing BlockDecl or null if there are no enclosing blocks.
+ const BlockDecl *getInnermostBlockDecl() const {
+const DeclContext *Ctx = this;
+
+do {
+ if (Ctx->isClosure())
+return cast(Ctx);
+ Ctx = Ctx->getParent();
+} while (Ctx);
+
+return nullptr;
+ }
+
bool isObjCContainer() const {
switch (getDeclKind()) {
case Decl::ObjCCategory:
Index: cfe/trunk/include/clang/Sema/Sema.h
===
--- cfe/trunk/include/clang/Sema/Sema.h
+++ cfe/trunk/include/clang/Sema/Sema.h
@@ -1213,6 +1213,11 @@
/// of -Wselector.
llvm::MapVector ReferencedSelectors;
+ /// List of SourceLocations where 'self' is implicitly retained inside a
+ /// block.
+ llvm::SmallVector, 1>
+ ImplicitlyRetainedSelfLocs;
+
/// Kinds of C++ special members.
enum CXXSpecialMember {
CXXDefaultConstructor,
Index: cfe/trunk/test/SemaObjCXX/warn-implicit-self-in-block.mm
===
--- cfe/trunk/test/SemaObjCXX/warn-implicit-self-in-block.mm
+++ cfe/trunk/test/SemaObjCXX/warn-implicit-self-in-block.mm
@@ -0,0 +1,42 @@
+// RUN: %clang_cc1 -x objective-c++ -std=c++11 -fobjc-arc -fblocks -Wimplicit-retain-self -verify %s
+// rdar://11194874
+
+typedef void (^BlockTy)();
+
+void noescapeFunc(__attribute__((noescape)) BlockTy);
+void escapeFunc(BlockTy);
+
+@interface Root @end
+
+@interface I : Root
+{
+ int _bar;
+}
+@end
+
+@implementation I
+ - (void)foo{
+ ^{
+ _bar = 3; // expected-warning {{block implicitly retains 'self'; explicitly mention 'self' to indicate this is intended behavior}}
+ }();
+ }
+
+ - (void)testNested{
+noescapeFunc(^{
+ (void)_bar;
+ escapeFunc(^{
+(void)_bar; // expected-warning {{block implicitly retains 'self'; explicitly mention 'self' to indicate this is intended behavior}}
+noescapeFunc(^{
+ (void)_bar; // expected-warning {{block implicitly retains 'self'; explicitly mention 'self' to indicate this is intended behavior}}
+});
+(void)_bar; // expected-warning {{block implicitly retains 'self'; explicitly mention 'self' to indicate this is intended behavior}}
+ });
+ (void)_bar;
+});
+ }
+
+ - (void)testLambdaInBlock{
+noescapeFunc(^{ [&](){ (void)_bar; }(); });
+escapeFunc(^{ [&](){ (void)_bar; }(); }); // expected-warning {{block implicitly retains 'self'; explicitly mention 'self' to indicate this is intended behavior}}
+ }
+@end
Index: cfe/trunk/lib/Sema/SemaExpr.cpp
===
--- cfe/trunk/lib/Sema/SemaExpr.cpp
+++ cfe/trunk/lib/Sema/SemaExpr.cpp
@@ -2575,11 +2575,9 @@
!Diags.isIgnored(diag::warn_arc_repeated_use_of_weak, Loc))
getCurFunction()->recordUseOfWeak(Result);
}
- if (getLangOpts().ObjCAutoRefCount) {
-if (CurContext->isClosure())
- Diag(Loc, diag::warn_implicitly_retains_self)
-<< FixItHint::CreateInsertion(Loc, "self->");
- }
+ if (getLangOpts().ObjCAutoRefCount)
+if (const BlockDecl *BD = CurContext->getInnermostBlockDecl())
+ ImplicitlyRetainedSelfLocs.push_back({Loc, BD});
return Result;
}
Index: cfe/trunk/lib/Sema/SemaDecl.cpp
===
--- cfe/trunk/lib/Sema/SemaDecl.cpp
+++ cfe/trunk/lib/Sema/SemaDecl.cpp
@@ -13150,6 +13150,35 @@
bool IsLambda = false;
};
+static void diagnoseImplicitlyRetainedSelf(Sema ) {
+ llvm::DenseMap EscapeInfo;
+
+ auto
[PATCH] D60736: [Sema][ObjC] Don't warn about a block implicitly retaining self if the block is marked noescape
erik.pilkington accepted this revision.
erik.pilkington added a comment.
LGTM too, thanks!
Comment at: lib/Sema/SemaDecl.cpp:13169
+do {
+ R = R || !CurBD->doesNotEscape();
+ if (R)
This can just be R = !CurBD->doesNotEscape();
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D60736/new/
https://reviews.llvm.org/D60736
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D60736: [Sema][ObjC] Don't warn about a block implicitly retaining self if the block is marked noescape
ahatanak updated this revision to Diff 195639.
ahatanak added a comment.
Diagnose implicitly retained self in `ActOnFinishFunctionBody`.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D60736/new/
https://reviews.llvm.org/D60736
Files:
include/clang/AST/DeclBase.h
include/clang/Sema/Sema.h
lib/Sema/SemaDecl.cpp
lib/Sema/SemaDeclObjC.cpp
lib/Sema/SemaExpr.cpp
test/SemaObjC/warn-implicit-self-in-block.m
test/SemaObjCXX/warn-implicit-self-in-block.mm
Index: test/SemaObjCXX/warn-implicit-self-in-block.mm
===
--- /dev/null
+++ test/SemaObjCXX/warn-implicit-self-in-block.mm
@@ -0,0 +1,42 @@
+// RUN: %clang_cc1 -x objective-c++ -std=c++11 -fobjc-arc -fblocks -Wimplicit-retain-self -verify %s
+// rdar://11194874
+
+typedef void (^BlockTy)();
+
+void noescapeFunc(__attribute__((noescape)) BlockTy);
+void escapeFunc(BlockTy);
+
+@interface Root @end
+
+@interface I : Root
+{
+ int _bar;
+}
+@end
+
+@implementation I
+ - (void)foo{
+ ^{
+ _bar = 3; // expected-warning {{block implicitly retains 'self'; explicitly mention 'self' to indicate this is intended behavior}}
+ }();
+ }
+
+ - (void)testNested{
+noescapeFunc(^{
+ (void)_bar;
+ escapeFunc(^{
+(void)_bar; // expected-warning {{block implicitly retains 'self'; explicitly mention 'self' to indicate this is intended behavior}}
+noescapeFunc(^{
+ (void)_bar; // expected-warning {{block implicitly retains 'self'; explicitly mention 'self' to indicate this is intended behavior}}
+});
+(void)_bar; // expected-warning {{block implicitly retains 'self'; explicitly mention 'self' to indicate this is intended behavior}}
+ });
+ (void)_bar;
+});
+ }
+
+ - (void)testLambdaInBlock{
+noescapeFunc(^{ [&](){ (void)_bar; }(); });
+escapeFunc(^{ [&](){ (void)_bar; }(); }); // expected-warning {{block implicitly retains 'self'; explicitly mention 'self' to indicate this is intended behavior}}
+ }
+@end
Index: test/SemaObjC/warn-implicit-self-in-block.m
===
--- test/SemaObjC/warn-implicit-self-in-block.m
+++ /dev/null
@@ -1,18 +0,0 @@
-// RUN: %clang_cc1 -x objective-c -fobjc-arc -fblocks -Wimplicit-retain-self -verify %s
-// rdar://11194874
-
-@interface Root @end
-
-@interface I : Root
-{
- int _bar;
-}
-@end
-
-@implementation I
- - (void)foo{
- ^{
- _bar = 3; // expected-warning {{block implicitly retains 'self'; explicitly mention 'self' to indicate this is intended behavior}}
- }();
- }
-@end
Index: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -2575,11 +2575,9 @@
!Diags.isIgnored(diag::warn_arc_repeated_use_of_weak, Loc))
getCurFunction()->recordUseOfWeak(Result);
}
- if (getLangOpts().ObjCAutoRefCount) {
-if (CurContext->isClosure())
- Diag(Loc, diag::warn_implicitly_retains_self)
-<< FixItHint::CreateInsertion(Loc, "self->");
- }
+ if (getLangOpts().ObjCAutoRefCount)
+if (const BlockDecl *BD = CurContext->getInnermostBlockDecl())
+ ImplicitlyRetainedSelfLocs.push_back({Loc, BD});
return Result;
}
Index: lib/Sema/SemaDeclObjC.cpp
===
--- lib/Sema/SemaDeclObjC.cpp
+++ lib/Sema/SemaDeclObjC.cpp
@@ -359,6 +359,7 @@
/// ActOnStartOfObjCMethodDef - This routine sets up parameters; invisible
/// and user declared, in the method definition's AST.
void Sema::ActOnStartOfObjCMethodDef(Scope *FnBodyScope, Decl *D) {
+ ImplicitlyRetainedSelfLocs.clear();
assert((getCurMethodDecl() == nullptr) && "Methodparsing confused");
ObjCMethodDecl *MDecl = dyn_cast_or_null(D);
Index: lib/Sema/SemaDecl.cpp
===
--- lib/Sema/SemaDecl.cpp
+++ lib/Sema/SemaDecl.cpp
@@ -13155,6 +13155,35 @@
bool IsLambda = false;
};
+static void diagnoseImplicitlyRetainedSelf(Sema ) {
+ llvm::DenseMap EscapeInfo;
+
+ auto IsOrNestedInEscapingBlock = [&](const BlockDecl *BD) {
+if (EscapeInfo.count(BD))
+ return EscapeInfo[BD];
+
+bool R = false;
+const BlockDecl *CurBD = BD;
+
+do {
+ R = R || !CurBD->doesNotEscape();
+ if (R)
+break;
+ CurBD = CurBD->getParent()->getInnermostBlockDecl();
+} while (CurBD);
+
+return EscapeInfo[BD] = R;
+ };
+
+ // If the location where 'self' is implicitly retained is inside a escaping
+ // block, emit a diagnostic.
+ for (const std::pair :
+ S.ImplicitlyRetainedSelfLocs)
+if (IsOrNestedInEscapingBlock(P.second))
+ S.Diag(P.first, diag::warn_implicitly_retains_self)
+ << FixItHint::CreateInsertion(P.first, "self->");
+}
+
Decl
[PATCH] D60736: [Sema][ObjC] Don't warn about a block implicitly retaining self if the block is marked noescape
rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Alright, LGTM. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60736/new/ https://reviews.llvm.org/D60736 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D60736: [Sema][ObjC] Don't warn about a block implicitly retaining self if the block is marked noescape
erik.pilkington added inline comments. Comment at: lib/Parse/ParseObjc.cpp:3696-3699 + + Actions.ActOnEndOfObjCMethodDef(); + // Clean up the remaining EOF token. Any reason not to do this check in `ActOnFinishFunctionBody` (which is called by ParseFunctionStatementBody)? Seems like there are some similar diagnostics implemented there. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60736/new/ https://reviews.llvm.org/D60736 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D60736: [Sema][ObjC] Don't warn about a block implicitly retaining self if the block is marked noescape
ahatanak updated this revision to Diff 195601.
ahatanak marked an inline comment as done.
ahatanak added a comment.
Rename function.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D60736/new/
https://reviews.llvm.org/D60736
Files:
include/clang/AST/DeclBase.h
include/clang/Sema/Sema.h
lib/Parse/ParseObjc.cpp
lib/Sema/SemaDeclObjC.cpp
lib/Sema/SemaExpr.cpp
test/SemaObjC/warn-implicit-self-in-block.m
test/SemaObjCXX/warn-implicit-self-in-block.mm
Index: test/SemaObjCXX/warn-implicit-self-in-block.mm
===
--- /dev/null
+++ test/SemaObjCXX/warn-implicit-self-in-block.mm
@@ -0,0 +1,42 @@
+// RUN: %clang_cc1 -x objective-c++ -std=c++11 -fobjc-arc -fblocks -Wimplicit-retain-self -verify %s
+// rdar://11194874
+
+typedef void (^BlockTy)();
+
+void noescapeFunc(__attribute__((noescape)) BlockTy);
+void escapeFunc(BlockTy);
+
+@interface Root @end
+
+@interface I : Root
+{
+ int _bar;
+}
+@end
+
+@implementation I
+ - (void)foo{
+ ^{
+ _bar = 3; // expected-warning {{block implicitly retains 'self'; explicitly mention 'self' to indicate this is intended behavior}}
+ }();
+ }
+
+ - (void)testNested{
+noescapeFunc(^{
+ (void)_bar;
+ escapeFunc(^{
+(void)_bar; // expected-warning {{block implicitly retains 'self'; explicitly mention 'self' to indicate this is intended behavior}}
+noescapeFunc(^{
+ (void)_bar; // expected-warning {{block implicitly retains 'self'; explicitly mention 'self' to indicate this is intended behavior}}
+});
+(void)_bar; // expected-warning {{block implicitly retains 'self'; explicitly mention 'self' to indicate this is intended behavior}}
+ });
+ (void)_bar;
+});
+ }
+
+ - (void)testLambdaInBlock{
+noescapeFunc(^{ [&](){ (void)_bar; }(); });
+escapeFunc(^{ [&](){ (void)_bar; }(); }); // expected-warning {{block implicitly retains 'self'; explicitly mention 'self' to indicate this is intended behavior}}
+ }
+@end
Index: test/SemaObjC/warn-implicit-self-in-block.m
===
--- test/SemaObjC/warn-implicit-self-in-block.m
+++ /dev/null
@@ -1,18 +0,0 @@
-// RUN: %clang_cc1 -x objective-c -fobjc-arc -fblocks -Wimplicit-retain-self -verify %s
-// rdar://11194874
-
-@interface Root @end
-
-@interface I : Root
-{
- int _bar;
-}
-@end
-
-@implementation I
- - (void)foo{
- ^{
- _bar = 3; // expected-warning {{block implicitly retains 'self'; explicitly mention 'self' to indicate this is intended behavior}}
- }();
- }
-@end
Index: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -2575,11 +2575,9 @@
!Diags.isIgnored(diag::warn_arc_repeated_use_of_weak, Loc))
getCurFunction()->recordUseOfWeak(Result);
}
- if (getLangOpts().ObjCAutoRefCount) {
-if (CurContext->isClosure())
- Diag(Loc, diag::warn_implicitly_retains_self)
-<< FixItHint::CreateInsertion(Loc, "self->");
- }
+ if (getLangOpts().ObjCAutoRefCount)
+if (const BlockDecl *BD = CurContext->getInnermostBlockDecl())
+ ImplicitlyRetainedSelfLocs.push_back({Loc, BD});
return Result;
}
Index: lib/Sema/SemaDeclObjC.cpp
===
--- lib/Sema/SemaDeclObjC.cpp
+++ lib/Sema/SemaDeclObjC.cpp
@@ -359,6 +359,7 @@
/// ActOnStartOfObjCMethodDef - This routine sets up parameters; invisible
/// and user declared, in the method definition's AST.
void Sema::ActOnStartOfObjCMethodDef(Scope *FnBodyScope, Decl *D) {
+ ImplicitlyRetainedSelfLocs.clear();
assert((getCurMethodDecl() == nullptr) && "Methodparsing confused");
ObjCMethodDecl *MDecl = dyn_cast_or_null(D);
@@ -494,6 +495,35 @@
}
}
+void Sema::ActOnEndOfObjCMethodDef() {
+ llvm::DenseMap EscapeInfo;
+
+ auto IsOrNestedInEscapingBlock = [&](const BlockDecl *BD) {
+if (EscapeInfo.count(BD))
+ return EscapeInfo[BD];
+
+bool R = false;
+const BlockDecl *CurBD = BD;
+
+do {
+ R = R || !CurBD->doesNotEscape();
+ if (R)
+break;
+ CurBD = CurBD->getParent()->getInnermostBlockDecl();
+} while (CurBD);
+
+return EscapeInfo[BD] = R;
+ };
+
+ // If the location where 'self' is implicitly retained is inside a escaping
+ // block, emit a diagnostic.
+ for (const std::pair :
+ ImplicitlyRetainedSelfLocs)
+if (IsOrNestedInEscapingBlock(P.second))
+ Diag(P.first, diag::warn_implicitly_retains_self)
+ << FixItHint::CreateInsertion(P.first, "self->");
+}
+
namespace {
// Callback to only accept typo corrections that are Objective-C classes.
Index: lib/Parse/ParseObjc.cpp
===
--- lib/Parse/ParseObjc.cpp
+++
[PATCH] D60736: [Sema][ObjC] Don't warn about a block implicitly retaining self if the block is marked noescape
rjmccall added inline comments.
Comment at: include/clang/AST/DeclBase.h:1798
+ /// innermost enclosing BlockDecl or null if there are no enclosing blocks.
+ const BlockDecl *getInnerMostBlockDecl() const {
+const DeclContext *Ctx = this;
"innermost" is one word, so this should be `getInnermostBlockDecl`.
Comment at: lib/Sema/SemaExpr.cpp:2580
+if (const BlockDecl *BD = CurContext->getInnerMostBlockDecl())
+ if (!getDiagnostics().isIgnored(diag::warn_implicitly_retains_self,
Loc))
+ImplicitlyRetainedSelfLocs.push_back({Loc, BD});
ahatanak wrote:
> rjmccall wrote:
> > IIRC this check can be expensive enough that it's probably not worth doing
> > if you expect these entries to typically not result in diagnostics.
> `DiagStateMap::lookup` is doing a binary search. Is that what makes this
> check expensive?
Yeah. It's not immensely expensive, but adding some entries to a vector in the
fast path and then processing them later probably makes more sense.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D60736/new/
https://reviews.llvm.org/D60736
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D60736: [Sema][ObjC] Don't warn about a block implicitly retaining self if the block is marked noescape
ahatanak added inline comments.
Comment at: lib/Sema/SemaExpr.cpp:2580
+if (const BlockDecl *BD = CurContext->getInnerMostBlockDecl())
+ if (!getDiagnostics().isIgnored(diag::warn_implicitly_retains_self,
Loc))
+ImplicitlyRetainedSelfLocs.push_back({Loc, BD});
rjmccall wrote:
> IIRC this check can be expensive enough that it's probably not worth doing if
> you expect these entries to typically not result in diagnostics.
`DiagStateMap::lookup` is doing a binary search. Is that what makes this check
expensive?
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D60736/new/
https://reviews.llvm.org/D60736
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D60736: [Sema][ObjC] Don't warn about a block implicitly retaining self if the block is marked noescape
ahatanak updated this revision to Diff 195589.
ahatanak marked 2 inline comments as done.
ahatanak added a comment.
Remove a potentially expensive check.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D60736/new/
https://reviews.llvm.org/D60736
Files:
include/clang/AST/DeclBase.h
include/clang/Sema/Sema.h
lib/Parse/ParseObjc.cpp
lib/Sema/SemaDeclObjC.cpp
lib/Sema/SemaExpr.cpp
test/SemaObjC/warn-implicit-self-in-block.m
test/SemaObjCXX/warn-implicit-self-in-block.mm
Index: test/SemaObjCXX/warn-implicit-self-in-block.mm
===
--- /dev/null
+++ test/SemaObjCXX/warn-implicit-self-in-block.mm
@@ -0,0 +1,42 @@
+// RUN: %clang_cc1 -x objective-c++ -std=c++11 -fobjc-arc -fblocks -Wimplicit-retain-self -verify %s
+// rdar://11194874
+
+typedef void (^BlockTy)();
+
+void noescapeFunc(__attribute__((noescape)) BlockTy);
+void escapeFunc(BlockTy);
+
+@interface Root @end
+
+@interface I : Root
+{
+ int _bar;
+}
+@end
+
+@implementation I
+ - (void)foo{
+ ^{
+ _bar = 3; // expected-warning {{block implicitly retains 'self'; explicitly mention 'self' to indicate this is intended behavior}}
+ }();
+ }
+
+ - (void)testNested{
+noescapeFunc(^{
+ (void)_bar;
+ escapeFunc(^{
+(void)_bar; // expected-warning {{block implicitly retains 'self'; explicitly mention 'self' to indicate this is intended behavior}}
+noescapeFunc(^{
+ (void)_bar; // expected-warning {{block implicitly retains 'self'; explicitly mention 'self' to indicate this is intended behavior}}
+});
+(void)_bar; // expected-warning {{block implicitly retains 'self'; explicitly mention 'self' to indicate this is intended behavior}}
+ });
+ (void)_bar;
+});
+ }
+
+ - (void)testLambdaInBlock{
+noescapeFunc(^{ [&](){ (void)_bar; }(); });
+escapeFunc(^{ [&](){ (void)_bar; }(); }); // expected-warning {{block implicitly retains 'self'; explicitly mention 'self' to indicate this is intended behavior}}
+ }
+@end
Index: test/SemaObjC/warn-implicit-self-in-block.m
===
--- test/SemaObjC/warn-implicit-self-in-block.m
+++ /dev/null
@@ -1,18 +0,0 @@
-// RUN: %clang_cc1 -x objective-c -fobjc-arc -fblocks -Wimplicit-retain-self -verify %s
-// rdar://11194874
-
-@interface Root @end
-
-@interface I : Root
-{
- int _bar;
-}
-@end
-
-@implementation I
- - (void)foo{
- ^{
- _bar = 3; // expected-warning {{block implicitly retains 'self'; explicitly mention 'self' to indicate this is intended behavior}}
- }();
- }
-@end
Index: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -2575,11 +2575,9 @@
!Diags.isIgnored(diag::warn_arc_repeated_use_of_weak, Loc))
getCurFunction()->recordUseOfWeak(Result);
}
- if (getLangOpts().ObjCAutoRefCount) {
-if (CurContext->isClosure())
- Diag(Loc, diag::warn_implicitly_retains_self)
-<< FixItHint::CreateInsertion(Loc, "self->");
- }
+ if (getLangOpts().ObjCAutoRefCount)
+if (const BlockDecl *BD = CurContext->getInnerMostBlockDecl())
+ ImplicitlyRetainedSelfLocs.push_back({Loc, BD});
return Result;
}
Index: lib/Sema/SemaDeclObjC.cpp
===
--- lib/Sema/SemaDeclObjC.cpp
+++ lib/Sema/SemaDeclObjC.cpp
@@ -359,6 +359,7 @@
/// ActOnStartOfObjCMethodDef - This routine sets up parameters; invisible
/// and user declared, in the method definition's AST.
void Sema::ActOnStartOfObjCMethodDef(Scope *FnBodyScope, Decl *D) {
+ ImplicitlyRetainedSelfLocs.clear();
assert((getCurMethodDecl() == nullptr) && "Methodparsing confused");
ObjCMethodDecl *MDecl = dyn_cast_or_null(D);
@@ -494,6 +495,35 @@
}
}
+void Sema::ActOnEndOfObjCMethodDef() {
+ llvm::DenseMap EscapeInfo;
+
+ auto IsOrNestedInEscapingBlock = [&](const BlockDecl *BD) {
+if (EscapeInfo.count(BD))
+ return EscapeInfo[BD];
+
+bool R = false;
+const BlockDecl *CurBD = BD;
+
+do {
+ R = R || !CurBD->doesNotEscape();
+ if (R)
+break;
+ CurBD = CurBD->getParent()->getInnerMostBlockDecl();
+} while (CurBD);
+
+return EscapeInfo[BD] = R;
+ };
+
+ // If the location where 'self' is implicitly retained is inside a escaping
+ // block, emit a diagnostic.
+ for (const std::pair :
+ ImplicitlyRetainedSelfLocs)
+if (IsOrNestedInEscapingBlock(P.second))
+ Diag(P.first, diag::warn_implicitly_retains_self)
+ << FixItHint::CreateInsertion(P.first, "self->");
+}
+
namespace {
// Callback to only accept typo corrections that are Objective-C classes.
Index: lib/Parse/ParseObjc.cpp
===
---
[PATCH] D60736: [Sema][ObjC] Don't warn about a block implicitly retaining self if the block is marked noescape
rjmccall added inline comments.
Comment at: lib/Sema/SemaExpr.cpp:2580
+if (const BlockDecl *BD = CurContext->getInnerMostBlockDecl())
+ if (!getDiagnostics().isIgnored(diag::warn_implicitly_retains_self,
Loc))
+ImplicitlyRetainedSelfLocs.push_back({Loc, BD});
IIRC this check can be expensive enough that it's probably not worth doing if
you expect these entries to typically not result in diagnostics.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D60736/new/
https://reviews.llvm.org/D60736
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D60736: [Sema][ObjC] Don't warn about a block implicitly retaining self if the block is marked noescape
ahatanak updated this revision to Diff 195498.
ahatanak added a comment.
Keep a list of pairs of BlockDecl and SourceLocation and, after the body of an
ObjC method is parsed, emit a diagnostic if a SourceLocation in the list is
inside an escaping block.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D60736/new/
https://reviews.llvm.org/D60736
Files:
include/clang/AST/DeclBase.h
include/clang/Sema/Sema.h
lib/Parse/ParseObjc.cpp
lib/Sema/SemaDeclObjC.cpp
lib/Sema/SemaExpr.cpp
test/SemaObjC/warn-implicit-self-in-block.m
test/SemaObjCXX/warn-implicit-self-in-block.mm
Index: test/SemaObjCXX/warn-implicit-self-in-block.mm
===
--- /dev/null
+++ test/SemaObjCXX/warn-implicit-self-in-block.mm
@@ -0,0 +1,42 @@
+// RUN: %clang_cc1 -x objective-c++ -std=c++11 -fobjc-arc -fblocks -Wimplicit-retain-self -verify %s
+// rdar://11194874
+
+typedef void (^BlockTy)();
+
+void noescapeFunc(__attribute__((noescape)) BlockTy);
+void escapeFunc(BlockTy);
+
+@interface Root @end
+
+@interface I : Root
+{
+ int _bar;
+}
+@end
+
+@implementation I
+ - (void)foo{
+ ^{
+ _bar = 3; // expected-warning {{block implicitly retains 'self'; explicitly mention 'self' to indicate this is intended behavior}}
+ }();
+ }
+
+ - (void)testNested{
+noescapeFunc(^{
+ (void)_bar;
+ escapeFunc(^{
+(void)_bar; // expected-warning {{block implicitly retains 'self'; explicitly mention 'self' to indicate this is intended behavior}}
+noescapeFunc(^{
+ (void)_bar; // expected-warning {{block implicitly retains 'self'; explicitly mention 'self' to indicate this is intended behavior}}
+});
+(void)_bar; // expected-warning {{block implicitly retains 'self'; explicitly mention 'self' to indicate this is intended behavior}}
+ });
+ (void)_bar;
+});
+ }
+
+ - (void)testLambdaInBlock{
+noescapeFunc(^{ [&](){ (void)_bar; }(); });
+escapeFunc(^{ [&](){ (void)_bar; }(); }); // expected-warning {{block implicitly retains 'self'; explicitly mention 'self' to indicate this is intended behavior}}
+ }
+@end
Index: test/SemaObjC/warn-implicit-self-in-block.m
===
--- test/SemaObjC/warn-implicit-self-in-block.m
+++ /dev/null
@@ -1,18 +0,0 @@
-// RUN: %clang_cc1 -x objective-c -fobjc-arc -fblocks -Wimplicit-retain-self -verify %s
-// rdar://11194874
-
-@interface Root @end
-
-@interface I : Root
-{
- int _bar;
-}
-@end
-
-@implementation I
- - (void)foo{
- ^{
- _bar = 3; // expected-warning {{block implicitly retains 'self'; explicitly mention 'self' to indicate this is intended behavior}}
- }();
- }
-@end
Index: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -2575,11 +2575,10 @@
!Diags.isIgnored(diag::warn_arc_repeated_use_of_weak, Loc))
getCurFunction()->recordUseOfWeak(Result);
}
- if (getLangOpts().ObjCAutoRefCount) {
-if (CurContext->isClosure())
- Diag(Loc, diag::warn_implicitly_retains_self)
-<< FixItHint::CreateInsertion(Loc, "self->");
- }
+ if (getLangOpts().ObjCAutoRefCount)
+if (const BlockDecl *BD = CurContext->getInnerMostBlockDecl())
+ if (!getDiagnostics().isIgnored(diag::warn_implicitly_retains_self, Loc))
+ImplicitlyRetainedSelfLocs.push_back({Loc, BD});
return Result;
}
Index: lib/Sema/SemaDeclObjC.cpp
===
--- lib/Sema/SemaDeclObjC.cpp
+++ lib/Sema/SemaDeclObjC.cpp
@@ -359,6 +359,7 @@
/// ActOnStartOfObjCMethodDef - This routine sets up parameters; invisible
/// and user declared, in the method definition's AST.
void Sema::ActOnStartOfObjCMethodDef(Scope *FnBodyScope, Decl *D) {
+ ImplicitlyRetainedSelfLocs.clear();
assert((getCurMethodDecl() == nullptr) && "Methodparsing confused");
ObjCMethodDecl *MDecl = dyn_cast_or_null(D);
@@ -494,6 +495,35 @@
}
}
+void Sema::ActOnEndOfObjCMethodDef() {
+ llvm::DenseMap EscapeInfo;
+
+ auto IsOrNestedInEscapingBlock = [&](const BlockDecl *BD) {
+if (EscapeInfo.count(BD))
+ return EscapeInfo[BD];
+
+bool R = false;
+const BlockDecl *CurBD = BD;
+
+do {
+ R = R || !CurBD->doesNotEscape();
+ if (R)
+break;
+ CurBD = CurBD->getParent()->getInnerMostBlockDecl();
+} while (CurBD);
+
+return EscapeInfo[BD] = R;
+ };
+
+ // If the location where 'self' is implicitly retained is inside a escaping
+ // block, emit a diagnostic.
+ for (const std::pair :
+ ImplicitlyRetainedSelfLocs)
+if (IsOrNestedInEscapingBlock(P.second))
+ Diag(P.first, diag::warn_implicitly_retains_self)
+ << FixItHint::CreateInsertion(P.first, "self->");
+}
+
namespace {
//
[PATCH] D60736: [Sema][ObjC] Don't warn about a block implicitly retaining self if the block is marked noescape
erik.pilkington added a comment.
Akira and I were just talking about an alternative approach to this: Keep a
vector of pairs of BlockDecls and SourceLocations in the enclosing method's
FunctionScopeInfo, and emit any unsuppressed diagnostics when popping the
method. This would avoid having to traverse all the blocks in methods in ARC
mode, at the cost of a small amount of memory.
Comment at: lib/Sema/Sema.cpp:1682-1687
+ void VisitBlockDecl(const BlockDecl *BD) {
+bool OldVisitingEscapingBlock = VisitingEscapingBlock;
+VisitingEscapingBlock = VisitingEscapingBlock || !BD->doesNotEscape();
+Visit(BD->getBody());
+VisitingEscapingBlock = OldVisitingEscapingBlock;
+ }
Maybe call this "diagnoseBlockDecl" or something so it doesn't sounds like a
CRTP-overridden method.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D60736/new/
https://reviews.llvm.org/D60736
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D60736: [Sema][ObjC] Don't warn about a block implicitly retaining self if the block is marked noescape
ahatanak added a comment. Sorry, clang was failing to diagnose `self` referenced inside a c++ lambda that was nested inside a block. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60736/new/ https://reviews.llvm.org/D60736 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D60736: [Sema][ObjC] Don't warn about a block implicitly retaining self if the block is marked noescape
ahatanak created this revision.
ahatanak added reviewers: rjmccall, erik.pilkington, arphaman.
ahatanak added a project: clang.
Herald added subscribers: dexonsmith, jkorous.
If the block implicitly referencing `self` doesn't escape, there is no risk of
creating retain cycles, so clang shouldn't diagnose it and force users to add
`self->` to silence the diagnostic.
Also, fix a bug where clang was failing to diagnose `self` referenced inside a
block that was nested inside a c++ lambda.
rdar://problem/25059955
Repository:
rC Clang
https://reviews.llvm.org/D60736
Files:
include/clang/Sema/ScopeInfo.h
lib/Sema/Sema.cpp
lib/Sema/SemaDeclObjC.cpp
lib/Sema/SemaExpr.cpp
test/SemaObjC/warn-implicit-self-in-block.m
test/SemaObjCXX/warn-implicit-self-in-block.mm
Index: test/SemaObjCXX/warn-implicit-self-in-block.mm
===
--- /dev/null
+++ test/SemaObjCXX/warn-implicit-self-in-block.mm
@@ -0,0 +1,42 @@
+// RUN: %clang_cc1 -x objective-c++ -std=c++11 -fobjc-arc -fblocks -Wimplicit-retain-self -verify %s
+// rdar://11194874
+
+typedef void (^BlockTy)();
+
+void noescapeFunc(__attribute__((noescape)) BlockTy);
+void escapeFunc(BlockTy);
+
+@interface Root @end
+
+@interface I : Root
+{
+ int _bar;
+}
+@end
+
+@implementation I
+ - (void)foo{
+ ^{
+ _bar = 3; // expected-warning {{block implicitly retains 'self'; explicitly mention 'self' to indicate this is intended behavior}}
+ }();
+ }
+
+ - (void)testNested{
+noescapeFunc(^{
+ (void)_bar;
+ escapeFunc(^{
+(void)_bar; // expected-warning {{block implicitly retains 'self'; explicitly mention 'self' to indicate this is intended behavior}}
+noescapeFunc(^{
+ (void)_bar; // expected-warning {{block implicitly retains 'self'; explicitly mention 'self' to indicate this is intended behavior}}
+});
+(void)_bar; // expected-warning {{block implicitly retains 'self'; explicitly mention 'self' to indicate this is intended behavior}}
+ });
+ (void)_bar;
+});
+ }
+
+ - (void)testLambdaInBlock{
+noescapeFunc(^{ [&](){ (void)_bar; }(); });
+escapeFunc(^{ [&](){ (void)_bar; }(); }); // expected-warning {{block implicitly retains 'self'; explicitly mention 'self' to indicate this is intended behavior}}
+ }
+@end
Index: test/SemaObjC/warn-implicit-self-in-block.m
===
--- test/SemaObjC/warn-implicit-self-in-block.m
+++ /dev/null
@@ -1,18 +0,0 @@
-// RUN: %clang_cc1 -x objective-c -fobjc-arc -fblocks -Wimplicit-retain-self -verify %s
-// rdar://11194874
-
-@interface Root @end
-
-@interface I : Root
-{
- int _bar;
-}
-@end
-
-@implementation I
- - (void)foo{
- ^{
- _bar = 3; // expected-warning {{block implicitly retains 'self'; explicitly mention 'self' to indicate this is intended behavior}}
- }();
- }
-@end
Index: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -2575,11 +2575,6 @@
!Diags.isIgnored(diag::warn_arc_repeated_use_of_weak, Loc))
getCurFunction()->recordUseOfWeak(Result);
}
- if (getLangOpts().ObjCAutoRefCount) {
-if (CurContext->isClosure())
- Diag(Loc, diag::warn_implicitly_retains_self)
-<< FixItHint::CreateInsertion(Loc, "self->");
- }
return Result;
}
@@ -13652,6 +13647,9 @@
}
}
+ if (getCurFunction())
+getCurFunction()->addBlock(Block, /*IsIntroducedInCurrentScope*/true);
+
PushBlockScope(CurScope, Block);
CurContext->addDecl(Block);
if (CurScope)
@@ -13913,9 +13911,6 @@
}
}
- if (getCurFunction())
-getCurFunction()->addBlock(BD);
-
return Result;
}
Index: lib/Sema/SemaDeclObjC.cpp
===
--- lib/Sema/SemaDeclObjC.cpp
+++ lib/Sema/SemaDeclObjC.cpp
@@ -378,6 +378,7 @@
// Allow all of Sema to see that we are entering a method definition.
PushDeclContext(FnBodyScope, MDecl);
PushFunctionScope();
+ getCurFunction()->setIsObjCMethod();
// Create Decl objects for each parameter, entrring them in the scope for
// binding to their use.
Index: lib/Sema/Sema.cpp
===
--- lib/Sema/Sema.cpp
+++ lib/Sema/Sema.cpp
@@ -20,6 +20,7 @@
#include "clang/AST/ExprCXX.h"
#include "clang/AST/PrettyDeclStackTrace.h"
#include "clang/AST/StmtCXX.h"
+#include "clang/AST/StmtVisitor.h"
#include "clang/Basic/DiagnosticOptions.h"
#include "clang/Basic/PartialDiagnostic.h"
#include "clang/Basic/TargetInfo.h"
@@ -1636,10 +1637,16 @@
static void markEscapingByrefs(const FunctionScopeInfo , Sema ) {
// Set the EscapingByref flag of __block variables captured by
// escaping blocks.
- for (const BlockDecl *BD : FSI.Blocks) {
-if (BD->doesNotEscape())
+
