[PATCH] D31005: [Objective-C] Fix "repeated use of weak" warning with -fobjc-weak

2017-03-27 Thread Brian T. Kelley via Phabricator via cfe-commits 
bkelley added a comment.

Thank you @rjmccall for the approval. I don't have commit access; would someone 
be willing to commit this path for me please? Thanks!


https://reviews.llvm.org/D31005



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D31005: [Objective-C] Fix "repeated use of weak" warning with -fobjc-weak

2017-03-25 Thread John McCall via Phabricator via cfe-commits 
rjmccall accepted this revision.
rjmccall added a comment.
This revision is now accepted and ready to land.

LGTM, thanks.


https://reviews.llvm.org/D31005



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D31005: [Objective-C] Fix "repeated use of weak" warning with -fobjc-weak

2017-03-24 Thread Brian T. Kelley via Phabricator via cfe-commits 
bkelley added a comment.

Sorry for missing the unnecessary LangOpts checks here. Thanks again for the 
feedback!


https://reviews.llvm.org/D31005



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D31005: [Objective-C] Fix "repeated use of weak" warning with -fobjc-weak

2017-03-24 Thread Brian T. Kelley via Phabricator via cfe-commits 
bkelley updated this revision to Diff 93023.
bkelley marked 4 inline comments as done.
bkelley added a comment.

Updated with feedback from @rjmccall


https://reviews.llvm.org/D31005

Files:
  include/clang/AST/Type.h
  lib/AST/Type.cpp
  lib/Sema/SemaDecl.cpp
  lib/Sema/SemaExpr.cpp
  lib/Sema/SemaExprMember.cpp
  lib/Sema/SemaExprObjC.cpp
  lib/Sema/SemaPseudoObject.cpp
  test/SemaObjC/arc-repeated-weak.mm

Index: test/SemaObjC/arc-repeated-weak.mm
===
--- test/SemaObjC/arc-repeated-weak.mm
+++ test/SemaObjC/arc-repeated-weak.mm
@@ -1,4 +1,5 @@
 // RUN: %clang_cc1 -fsyntax-only -fobjc-runtime-has-weak -fobjc-arc -fblocks -Wno-objc-root-class -std=c++11 -Warc-repeated-use-of-weak -verify %s
+// RUN: %clang_cc1 -fsyntax-only -fobjc-runtime-has-weak -fobjc-weak -fblocks -Wno-objc-root-class -std=c++11 -Warc-repeated-use-of-weak -verify %s
 
 @interface Test {
 @public
@@ -445,8 +446,8 @@
 @class NSString;
 @interface NSBundle
 +(NSBundle *)foo;
-@property (class) NSBundle *foo2;
-@property NSString *prop;
+@property (class, strong) NSBundle *foo2;
+@property (strong) NSString *prop;
 @property(weak) NSString *weakProp;
 @end
 
@@ -473,5 +474,8 @@
 };
 
 void foo1() {
-  INTFPtrTy tmp = (INTFPtrTy)e1; // expected-error{{cast of 'E' to 'INTFPtrTy' (aka 'INTF *') is disallowed with ARC}}
+  INTFPtrTy tmp = (INTFPtrTy)e1;
+#if __has_feature(objc_arc)
+// expected-error@-2{{cast of 'E' to 'INTFPtrTy' (aka 'INTF *') is disallowed with ARC}}
+#endif
 }
Index: lib/Sema/SemaPseudoObject.cpp
===
--- lib/Sema/SemaPseudoObject.cpp
+++ lib/Sema/SemaPseudoObject.cpp
@@ -841,12 +841,10 @@
   result = S.ImpCastExprToType(result.get(), propType, CK_BitCast);
   }
 }
-if (S.getLangOpts().ObjCAutoRefCount) {
-  Qualifiers::ObjCLifetime LT = propType.getObjCLifetime();
-  if (LT == Qualifiers::OCL_Weak)
-if (!S.Diags.isIgnored(diag::warn_arc_repeated_use_of_weak, RefExpr->getLocation()))
-  S.getCurFunction()->markSafeWeakUse(RefExpr);
-}
+if (propType.getObjCLifetime() == Qualifiers::OCL_Weak &&
+!S.Diags.isIgnored(diag::warn_arc_repeated_use_of_weak,
+   RefExpr->getLocation()))
+  S.getCurFunction()->markSafeWeakUse(RefExpr);
   }
 
   return result;
@@ -962,11 +960,11 @@
 }
 
 ExprResult ObjCPropertyOpBuilder::complete(Expr *SyntacticForm) {
-  if (S.getLangOpts().ObjCAutoRefCount && isWeakProperty() &&
+  if (isWeakProperty() &&
   !S.Diags.isIgnored(diag::warn_arc_repeated_use_of_weak,
  SyntacticForm->getLocStart()))
-  S.recordUseOfEvaluatedWeak(SyntacticRefExpr,
- SyntacticRefExpr->isMessagingGetter());
+S.recordUseOfEvaluatedWeak(SyntacticRefExpr,
+   SyntacticRefExpr->isMessagingGetter());
 
   return PseudoOpBuilder::complete(SyntacticForm);
 }
Index: lib/Sema/SemaExprObjC.cpp
===
--- lib/Sema/SemaExprObjC.cpp
+++ lib/Sema/SemaExprObjC.cpp
@@ -3100,7 +3100,9 @@
 // In ARC, check for message sends which are likely to introduce
 // retain cycles.
 checkRetainCycles(Result);
+  }
 
+  if (getLangOpts().ObjCWeak) {
 if (!isImplicit && Method) {
   if (const ObjCPropertyDecl *Prop = Method->findPropertyDecl()) {
 bool IsWeak =
Index: lib/Sema/SemaExprMember.cpp
===
--- lib/Sema/SemaExprMember.cpp
+++ lib/Sema/SemaExprMember.cpp
@@ -1475,7 +1475,7 @@
   }
 }
 bool warn = true;
-if (S.getLangOpts().ObjCAutoRefCount) {
+if (S.getLangOpts().ObjCWeak) {
   Expr *BaseExp = BaseExpr.get()->IgnoreParenImpCasts();
   if (UnaryOperator *UO = dyn_cast(BaseExp))
 if (UO->getOpcode() == UO_Deref)
@@ -1502,11 +1502,9 @@
 IV, IV->getUsageType(BaseType), MemberLoc, OpLoc, BaseExpr.get(),
 IsArrow);
 
-if (S.getLangOpts().ObjCAutoRefCount) {
-  if (IV->getType().getObjCLifetime() == Qualifiers::OCL_Weak) {
-if (!S.Diags.isIgnored(diag::warn_arc_repeated_use_of_weak, MemberLoc))
-  S.recordUseOfEvaluatedWeak(Result);
-  }
+if (IV->getType().getObjCLifetime() == Qualifiers::OCL_Weak) {
+  if (!S.Diags.isIgnored(diag::warn_arc_repeated_use_of_weak, MemberLoc))
+S.recordUseOfEvaluatedWeak(Result);
 }
 
 return Result;
Index: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -704,8 +704,7 @@
   
   // Loading a __weak object implicitly retains the value, so we need a cleanup to 
   // balance that.
-  if (getLangOpts().ObjCAutoRefCount &&
-  E->getType().getObjCLifetime() == Qualifiers::OCL_Weak)
+  if (E->getType().getObjCLifetime() == Qualifiers::OCL_Weak)

[PATCH] D31005: [Objective-C] Fix "repeated use of weak" warning with -fobjc-weak

2017-03-24 Thread John McCall via Phabricator via cfe-commits 
rjmccall added a comment.

Just a couple of minor requests.




Comment at: lib/Sema/SemaExpr.cpp:708
+  if (getLangOpts().ObjCWeak &&
   E->getType().getObjCLifetime() == Qualifiers::OCL_Weak)
 Cleanup.setExprNeedsCleanups(true);

Much like the other patches, it's probably more efficient to just check the 
qualifier here instead of testing the language option first.



Comment at: lib/Sema/SemaExpr.cpp:2513
+  if (getLangOpts().ObjCWeak) {
 if (IV->getType().getObjCLifetime() == Qualifiers::OCL_Weak) {
   if (!Diags.isIgnored(diag::warn_arc_repeated_use_of_weak, Loc))

Same thing.



Comment at: lib/Sema/SemaExprMember.cpp:1506
+if (S.getLangOpts().ObjCWeak) {
   if (IV->getType().getObjCLifetime() == Qualifiers::OCL_Weak) {
 if (!S.Diags.isIgnored(diag::warn_arc_repeated_use_of_weak, MemberLoc))

Same.



Comment at: lib/Sema/SemaPseudoObject.cpp:846
   Qualifiers::ObjCLifetime LT = propType.getObjCLifetime();
   if (LT == Qualifiers::OCL_Weak)
 if (!S.Diags.isIgnored(diag::warn_arc_repeated_use_of_weak, 
RefExpr->getLocation()))

Same.


https://reviews.llvm.org/D31005



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D31005: [Objective-C] Fix "repeated use of weak" warning with -fobjc-weak

2017-03-24 Thread Brian T. Kelley via Phabricator via cfe-commits 
bkelley added a comment.

Thanks again for the feedback. Is there anything further I should update in 
this diff or is it looking good?

Thanks!
Brian


https://reviews.llvm.org/D31005



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D31005: [Objective-C] Fix "repeated use of weak" warning with -fobjc-weak

2017-03-17 Thread Brian T. Kelley via Phabricator via cfe-commits 
bkelley updated this revision to Diff 92218.
bkelley added a comment.

Updated with feedback from @jordan_rose and @arphaman


https://reviews.llvm.org/D31005

Files:
  include/clang/AST/Type.h
  lib/AST/Type.cpp
  lib/Sema/SemaDecl.cpp
  lib/Sema/SemaExpr.cpp
  lib/Sema/SemaExprMember.cpp
  lib/Sema/SemaExprObjC.cpp
  lib/Sema/SemaPseudoObject.cpp
  test/SemaObjC/arc-repeated-weak.mm

Index: test/SemaObjC/arc-repeated-weak.mm
===
--- test/SemaObjC/arc-repeated-weak.mm
+++ test/SemaObjC/arc-repeated-weak.mm
@@ -1,4 +1,5 @@
 // RUN: %clang_cc1 -fsyntax-only -fobjc-runtime-has-weak -fobjc-arc -fblocks -Wno-objc-root-class -std=c++11 -Warc-repeated-use-of-weak -verify %s
+// RUN: %clang_cc1 -fsyntax-only -fobjc-runtime-has-weak -fobjc-weak -fblocks -Wno-objc-root-class -std=c++11 -Warc-repeated-use-of-weak -verify %s
 
 @interface Test {
 @public
@@ -445,8 +446,8 @@
 @class NSString;
 @interface NSBundle
 +(NSBundle *)foo;
-@property (class) NSBundle *foo2;
-@property NSString *prop;
+@property (class, strong) NSBundle *foo2;
+@property (strong) NSString *prop;
 @property(weak) NSString *weakProp;
 @end
 
@@ -473,5 +474,8 @@
 };
 
 void foo1() {
-  INTFPtrTy tmp = (INTFPtrTy)e1; // expected-error{{cast of 'E' to 'INTFPtrTy' (aka 'INTF *') is disallowed with ARC}}
+  INTFPtrTy tmp = (INTFPtrTy)e1;
+#if __has_feature(objc_arc)
+// expected-error@-2{{cast of 'E' to 'INTFPtrTy' (aka 'INTF *') is disallowed with ARC}}
+#endif
 }
Index: lib/Sema/SemaPseudoObject.cpp
===
--- lib/Sema/SemaPseudoObject.cpp
+++ lib/Sema/SemaPseudoObject.cpp
@@ -841,7 +841,7 @@
   result = S.ImpCastExprToType(result.get(), propType, CK_BitCast);
   }
 }
-if (S.getLangOpts().ObjCAutoRefCount) {
+if (S.getLangOpts().ObjCWeak) {
   Qualifiers::ObjCLifetime LT = propType.getObjCLifetime();
   if (LT == Qualifiers::OCL_Weak)
 if (!S.Diags.isIgnored(diag::warn_arc_repeated_use_of_weak, RefExpr->getLocation()))
@@ -962,11 +962,11 @@
 }
 
 ExprResult ObjCPropertyOpBuilder::complete(Expr *SyntacticForm) {
-  if (S.getLangOpts().ObjCAutoRefCount && isWeakProperty() &&
+  if (S.getLangOpts().ObjCWeak && isWeakProperty() &&
   !S.Diags.isIgnored(diag::warn_arc_repeated_use_of_weak,
  SyntacticForm->getLocStart()))
-  S.recordUseOfEvaluatedWeak(SyntacticRefExpr,
- SyntacticRefExpr->isMessagingGetter());
+S.recordUseOfEvaluatedWeak(SyntacticRefExpr,
+   SyntacticRefExpr->isMessagingGetter());
 
   return PseudoOpBuilder::complete(SyntacticForm);
 }
Index: lib/Sema/SemaExprObjC.cpp
===
--- lib/Sema/SemaExprObjC.cpp
+++ lib/Sema/SemaExprObjC.cpp
@@ -3100,7 +3100,9 @@
 // In ARC, check for message sends which are likely to introduce
 // retain cycles.
 checkRetainCycles(Result);
+  }
 
+  if (getLangOpts().ObjCWeak) {
 if (!isImplicit && Method) {
   if (const ObjCPropertyDecl *Prop = Method->findPropertyDecl()) {
 bool IsWeak =
Index: lib/Sema/SemaExprMember.cpp
===
--- lib/Sema/SemaExprMember.cpp
+++ lib/Sema/SemaExprMember.cpp
@@ -1475,7 +1475,7 @@
   }
 }
 bool warn = true;
-if (S.getLangOpts().ObjCAutoRefCount) {
+if (S.getLangOpts().ObjCWeak) {
   Expr *BaseExp = BaseExpr.get()->IgnoreParenImpCasts();
   if (UnaryOperator *UO = dyn_cast(BaseExp))
 if (UO->getOpcode() == UO_Deref)
@@ -1502,7 +1502,7 @@
 IV, IV->getUsageType(BaseType), MemberLoc, OpLoc, BaseExpr.get(),
 IsArrow);
 
-if (S.getLangOpts().ObjCAutoRefCount) {
+if (S.getLangOpts().ObjCWeak) {
   if (IV->getType().getObjCLifetime() == Qualifiers::OCL_Weak) {
 if (!S.Diags.isIgnored(diag::warn_arc_repeated_use_of_weak, MemberLoc))
   S.recordUseOfEvaluatedWeak(Result);
Index: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -704,7 +704,7 @@
   
   // Loading a __weak object implicitly retains the value, so we need a cleanup to 
   // balance that.
-  if (getLangOpts().ObjCAutoRefCount &&
+  if (getLangOpts().ObjCWeak &&
   E->getType().getObjCLifetime() == Qualifiers::OCL_Weak)
 Cleanup.setExprNeedsCleanups(true);
 
@@ -2509,11 +2509,13 @@
   ObjCIvarRefExpr(IV, IV->getUsageType(SelfExpr.get()->getType()), Loc,
   IV->getLocation(), SelfExpr.get(), true, true);
 
-  if (getLangOpts().ObjCAutoRefCount) {
+  if (getLangOpts().ObjCWeak) {
 if (IV->getType().getObjCLifetime() == Qualifiers::OCL_Weak) {
   if (!Diags.isIgnored(diag::warn_arc_repeated_use_of_weak, Loc))
 recordUseOfEvaluatedWeak(Result);
 }
+  }
+  if

[PATCH] D31005: [Objective-C] Fix "repeated use of weak" warning with -fobjc-weak

2017-03-17 Thread Brian T. Kelley via Phabricator via cfe-commits 
bkelley marked 3 inline comments as done.
bkelley added inline comments.



Comment at: lib/Sema/SemaDecl.cpp:10184
+ (!getLangOpts().ObjCAutoRefCount && getLangOpts().ObjCWeak &&
+  VDecl->getType().getObjCLifetime() != Qualifiers::OCL_Weak)) &&
 !Diags.isIgnored(diag::warn_arc_repeated_use_of_weak,

arphaman wrote:
> jordan_rose wrote:
> > bkelley wrote:
> > > jordan_rose wrote:
> > > > This condition's getting complicated, and it shows up in a few places. 
> > > > Would it make sense to factor it out?
> > > What do you think about adding a member function like 
> > > `hasMRCNonTrivialWeakObjCLifetime(const ASTContext )` to QualType 
> > > to factor out lines 10183-10184? We could use that in D31003, D31004, 
> > > here, and D31007.
> > I'm fine with it myself but I don't work on Clang very much anymore. Maybe 
> > someone else can say whether it's actually a good idea.
> > 
> > (By the way, the conventional abbreviation for this mode is "MRR" for 
> > "Manual Retain/Release", even though it's "ARC" and "Automated Reference 
> > Counting".)
> Do you want to extract the out the entire
> 
> ```
> (!getLangOpts().ObjCAutoRefCount && getLangOpts().ObjCWeak &&
>   VDecl->getType().getObjCLifetime() != Qualifiers::OCL_Weak)
> ```
> ?
> 
> It looks like the others patches use only `getLangOpts().ObjCWeak && 
> Type.getObjCLifetime() != Qualifiers::OCL_Weak` so the use of 
> `hasMRCNonTrivialWeakObjCLifetime` won't be equivalent to the original code 
> in the other patches if you extract all code from lines 10183-10184.
Yeah, my suspicion was that the addition of `!getLangOpts().ObjCAutoRefCount()` 
would have been fine, but most of the other code was simplified by using 
`hasNonTrivialObjCLifetime()` or another means, so this new function seems to 
only be necessary in this patch. I misnamed the proposed function, which would 
imply the qualifier is `OCL_Weak`, but we need //not// that, so my new proposed 
name is the odd looking `isNonWeakInMRRWithObjCWeak()`.



Comment at: lib/Sema/SemaExpr.cpp:10340
 
-  } else if (getLangOpts().ObjCAutoRefCount) {
+  } else if (getLangOpts().ObjCAutoRefCount || getLangOpts().ObjCWeak) {
 checkUnsafeExprAssigns(Loc, LHSExpr, RHS.get());

jordan_rose wrote:
> bkelley wrote:
> > jordan_rose wrote:
> > > Does ObjCAutoRefCount imply ObjCWeak? If so, you could just use the 
> > > latter.
> > I don't believe so. For Snow Leopard, ARC without weak references was 
> > supported so they can be independent. 
> Sure, but in that case we don't need the warning, right?
Oh, I see. Yeah, looks like I can update most of the checks to just use 
ObjCWeak. I think we need both conditions here, however, since 
`checkUnsafeExprAssigns()` emits the "assigning retained object to unsafe 
property" warning, which is only applicable in ARC.


https://reviews.llvm.org/D31005



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D31005: [Objective-C] Fix "repeated use of weak" warning with -fobjc-weak

2017-03-16 Thread Alex Lorenz via Phabricator via cfe-commits 
arphaman added inline comments.



Comment at: lib/Sema/SemaDecl.cpp:10184
+ (!getLangOpts().ObjCAutoRefCount && getLangOpts().ObjCWeak &&
+  VDecl->getType().getObjCLifetime() != Qualifiers::OCL_Weak)) &&
 !Diags.isIgnored(diag::warn_arc_repeated_use_of_weak,

jordan_rose wrote:
> bkelley wrote:
> > jordan_rose wrote:
> > > This condition's getting complicated, and it shows up in a few places. 
> > > Would it make sense to factor it out?
> > What do you think about adding a member function like 
> > `hasMRCNonTrivialWeakObjCLifetime(const ASTContext )` to QualType 
> > to factor out lines 10183-10184? We could use that in D31003, D31004, here, 
> > and D31007.
> I'm fine with it myself but I don't work on Clang very much anymore. Maybe 
> someone else can say whether it's actually a good idea.
> 
> (By the way, the conventional abbreviation for this mode is "MRR" for "Manual 
> Retain/Release", even though it's "ARC" and "Automated Reference Counting".)
Do you want to extract the out the entire

```
(!getLangOpts().ObjCAutoRefCount && getLangOpts().ObjCWeak &&
  VDecl->getType().getObjCLifetime() != Qualifiers::OCL_Weak)
```
?

It looks like the others patches use only `getLangOpts().ObjCWeak && 
Type.getObjCLifetime() != Qualifiers::OCL_Weak` so the use of 
`hasMRCNonTrivialWeakObjCLifetime` won't be equivalent to the original code in 
the other patches if you extract all code from lines 10183-10184.



Comment at: test/SemaObjC/arc-repeated-weak.mm:468
+// With -fobjc-weak, the cast below is allowed.
+#if __has_feature(objc_arc)
 // This used to crash in the constructor of WeakObjectProfileTy when a

NIT: You can keep the code in `foo1` active and just use the `#if` on the 
`expected-error`, like:

```
void foo1() {
  INTFPtrTy tmp = (INTFPtrTy)e1; 
#if __has_feature(objc_arc)
// expected-error@-2 {{cast of 'E' to 'INTFPtrTy' (aka 'INTF *') is disallowed 
with ARC}}
#endif
}
```


https://reviews.llvm.org/D31005



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D31005: [Objective-C] Fix "repeated use of weak" warning with -fobjc-weak

2017-03-15 Thread Jordan Rose via Phabricator via cfe-commits 
jordan_rose added inline comments.



Comment at: lib/Sema/SemaDecl.cpp:10184
+ (!getLangOpts().ObjCAutoRefCount && getLangOpts().ObjCWeak &&
+  VDecl->getType().getObjCLifetime() != Qualifiers::OCL_Weak)) &&
 !Diags.isIgnored(diag::warn_arc_repeated_use_of_weak,

bkelley wrote:
> jordan_rose wrote:
> > This condition's getting complicated, and it shows up in a few places. 
> > Would it make sense to factor it out?
> What do you think about adding a member function like 
> `hasMRCNonTrivialWeakObjCLifetime(const ASTContext )` to QualType to 
> factor out lines 10183-10184? We could use that in D31003, D31004, here, and 
> D31007.
I'm fine with it myself but I don't work on Clang very much anymore. Maybe 
someone else can say whether it's actually a good idea.

(By the way, the conventional abbreviation for this mode is "MRR" for "Manual 
Retain/Release", even though it's "ARC" and "Automated Reference Counting".)



Comment at: lib/Sema/SemaExpr.cpp:10340
 
-  } else if (getLangOpts().ObjCAutoRefCount) {
+  } else if (getLangOpts().ObjCAutoRefCount || getLangOpts().ObjCWeak) {
 checkUnsafeExprAssigns(Loc, LHSExpr, RHS.get());

bkelley wrote:
> jordan_rose wrote:
> > Does ObjCAutoRefCount imply ObjCWeak? If so, you could just use the latter.
> I don't believe so. For Snow Leopard, ARC without weak references was 
> supported so they can be independent. 
Sure, but in that case we don't need the warning, right?


https://reviews.llvm.org/D31005



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D31005: [Objective-C] Fix "repeated use of weak" warning with -fobjc-weak

2017-03-15 Thread Brian T. Kelley via Phabricator via cfe-commits 
bkelley added inline comments.



Comment at: lib/Sema/SemaDecl.cpp:10184
+ (!getLangOpts().ObjCAutoRefCount && getLangOpts().ObjCWeak &&
+  VDecl->getType().getObjCLifetime() != Qualifiers::OCL_Weak)) &&
 !Diags.isIgnored(diag::warn_arc_repeated_use_of_weak,

jordan_rose wrote:
> This condition's getting complicated, and it shows up in a few places. Would 
> it make sense to factor it out?
What do you think about adding a member function like 
`hasMRCNonTrivialWeakObjCLifetime(const ASTContext )` to QualType to 
factor out lines 10183-10184? We could use that in D31003, D31004, here, and 
D31007.



Comment at: lib/Sema/SemaExpr.cpp:10340
 
-  } else if (getLangOpts().ObjCAutoRefCount) {
+  } else if (getLangOpts().ObjCAutoRefCount || getLangOpts().ObjCWeak) {
 checkUnsafeExprAssigns(Loc, LHSExpr, RHS.get());

jordan_rose wrote:
> Does ObjCAutoRefCount imply ObjCWeak? If so, you could just use the latter.
I don't believe so. For Snow Leopard, ARC without weak references was supported 
so they can be independent. 


https://reviews.llvm.org/D31005



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D31005: [Objective-C] Fix "repeated use of weak" warning with -fobjc-weak

2017-03-15 Thread Jordan Rose via Phabricator via cfe-commits 
jordan_rose added a reviewer: rjmccall.
jordan_rose added a subscriber: rjmccall.
jordan_rose added a comment.

The warning-related parts look reasonable to me.




Comment at: lib/Sema/SemaDecl.cpp:10184
+ (!getLangOpts().ObjCAutoRefCount && getLangOpts().ObjCWeak &&
+  VDecl->getType().getObjCLifetime() != Qualifiers::OCL_Weak)) &&
 !Diags.isIgnored(diag::warn_arc_repeated_use_of_weak,

This condition's getting complicated, and it shows up in a few places. Would it 
make sense to factor it out?



Comment at: lib/Sema/SemaExpr.cpp:707
   // balance that.
-  if (getLangOpts().ObjCAutoRefCount &&
+  if ((getLangOpts().ObjCAutoRefCount || getLangOpts().ObjCWeak) &&
   E->getType().getObjCLifetime() == Qualifiers::OCL_Weak)

This is the one section that //isn't// dealing with the warning. I'd like 
@rjmccall to verify that it's correct.



Comment at: lib/Sema/SemaExpr.cpp:10340
 
-  } else if (getLangOpts().ObjCAutoRefCount) {
+  } else if (getLangOpts().ObjCAutoRefCount || getLangOpts().ObjCWeak) {
 checkUnsafeExprAssigns(Loc, LHSExpr, RHS.get());

Does ObjCAutoRefCount imply ObjCWeak? If so, you could just use the latter.


https://reviews.llvm.org/D31005



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D31005: [Objective-C] Fix "repeated use of weak" warning with -fobjc-weak

2017-03-15 Thread Brian T. Kelley via Phabricator via cfe-commits 
bkelley created this revision.

-Warc-repeated-use-of-weak should produce the same warnings with -fobjc-weak as 
it does with -objc-arc. Also check for ObjCWeak along with ObjCAutoRefCount 
when recording the use of an evaluated weak variable. Add a -fobjc-weak run to 
the existing arc-repeated-weak test case and adapt it slightly to work in both 
modes.


https://reviews.llvm.org/D31005

Files:
  lib/Sema/SemaDecl.cpp
  lib/Sema/SemaExpr.cpp
  lib/Sema/SemaExprMember.cpp
  lib/Sema/SemaExprObjC.cpp
  lib/Sema/SemaPseudoObject.cpp
  test/SemaObjC/arc-repeated-weak.mm

Index: test/SemaObjC/arc-repeated-weak.mm
===
--- test/SemaObjC/arc-repeated-weak.mm
+++ test/SemaObjC/arc-repeated-weak.mm
@@ -1,4 +1,5 @@
 // RUN: %clang_cc1 -fsyntax-only -fobjc-runtime-has-weak -fobjc-arc -fblocks -Wno-objc-root-class -std=c++11 -Warc-repeated-use-of-weak -verify %s
+// RUN: %clang_cc1 -fsyntax-only -fobjc-runtime-has-weak -fobjc-weak -fblocks -Wno-objc-root-class -std=c++11 -Warc-repeated-use-of-weak -verify %s
 
 @interface Test {
 @public
@@ -445,8 +446,8 @@
 @class NSString;
 @interface NSBundle
 +(NSBundle *)foo;
-@property (class) NSBundle *foo2;
-@property NSString *prop;
+@property (class, strong) NSBundle *foo2;
+@property (strong) NSString *prop;
 @property(weak) NSString *weakProp;
 @end
 
@@ -463,6 +464,8 @@
   use(NSBundle2.foo2.weakProp); // expected-note{{also accessed here}}
 }
 
+// With -fobjc-weak, the cast below is allowed.
+#if __has_feature(objc_arc)
 // This used to crash in the constructor of WeakObjectProfileTy when a
 // DeclRefExpr was passed that didn't reference a VarDecl.
 
@@ -475,3 +478,4 @@
 void foo1() {
   INTFPtrTy tmp = (INTFPtrTy)e1; // expected-error{{cast of 'E' to 'INTFPtrTy' (aka 'INTF *') is disallowed with ARC}}
 }
+#endif
Index: lib/Sema/SemaPseudoObject.cpp
===
--- lib/Sema/SemaPseudoObject.cpp
+++ lib/Sema/SemaPseudoObject.cpp
@@ -841,7 +841,7 @@
   result = S.ImpCastExprToType(result.get(), propType, CK_BitCast);
   }
 }
-if (S.getLangOpts().ObjCAutoRefCount) {
+if (S.getLangOpts().ObjCAutoRefCount || S.getLangOpts().ObjCWeak) {
   Qualifiers::ObjCLifetime LT = propType.getObjCLifetime();
   if (LT == Qualifiers::OCL_Weak)
 if (!S.Diags.isIgnored(diag::warn_arc_repeated_use_of_weak, RefExpr->getLocation()))
@@ -962,11 +962,12 @@
 }
 
 ExprResult ObjCPropertyOpBuilder::complete(Expr *SyntacticForm) {
-  if (S.getLangOpts().ObjCAutoRefCount && isWeakProperty() &&
+  if ((S.getLangOpts().ObjCAutoRefCount || S.getLangOpts().ObjCWeak) &&
+  isWeakProperty() &&
   !S.Diags.isIgnored(diag::warn_arc_repeated_use_of_weak,
  SyntacticForm->getLocStart()))
-  S.recordUseOfEvaluatedWeak(SyntacticRefExpr,
- SyntacticRefExpr->isMessagingGetter());
+S.recordUseOfEvaluatedWeak(SyntacticRefExpr,
+   SyntacticRefExpr->isMessagingGetter());
 
   return PseudoOpBuilder::complete(SyntacticForm);
 }
Index: lib/Sema/SemaExprObjC.cpp
===
--- lib/Sema/SemaExprObjC.cpp
+++ lib/Sema/SemaExprObjC.cpp
@@ -3100,7 +3100,9 @@
 // In ARC, check for message sends which are likely to introduce
 // retain cycles.
 checkRetainCycles(Result);
+  }
 
+  if (getLangOpts().ObjCAutoRefCount || getLangOpts().ObjCWeak) {
 if (!isImplicit && Method) {
   if (const ObjCPropertyDecl *Prop = Method->findPropertyDecl()) {
 bool IsWeak =
Index: lib/Sema/SemaExprMember.cpp
===
--- lib/Sema/SemaExprMember.cpp
+++ lib/Sema/SemaExprMember.cpp
@@ -1475,7 +1475,7 @@
   }
 }
 bool warn = true;
-if (S.getLangOpts().ObjCAutoRefCount) {
+if (S.getLangOpts().ObjCAutoRefCount || S.getLangOpts().ObjCWeak) {
   Expr *BaseExp = BaseExpr.get()->IgnoreParenImpCasts();
   if (UnaryOperator *UO = dyn_cast(BaseExp))
 if (UO->getOpcode() == UO_Deref)
@@ -1502,7 +1502,7 @@
 IV, IV->getUsageType(BaseType), MemberLoc, OpLoc, BaseExpr.get(),
 IsArrow);
 
-if (S.getLangOpts().ObjCAutoRefCount) {
+if (S.getLangOpts().ObjCAutoRefCount || S.getLangOpts().ObjCWeak) {
   if (IV->getType().getObjCLifetime() == Qualifiers::OCL_Weak) {
 if (!S.Diags.isIgnored(diag::warn_arc_repeated_use_of_weak, MemberLoc))
   S.recordUseOfEvaluatedWeak(Result);
Index: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -704,7 +704,7 @@
   
   // Loading a __weak object implicitly retains the value, so we need a cleanup to 
   // balance that.
-  if (getLangOpts().ObjCAutoRefCount &&
+  if ((getLangOpts().ObjCAutoRefCount || getLangOpts().ObjCWeak) &&