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

[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 &Context)` 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 at -2 {{cast of 'E' to 'INTFPtrTy' (aka 'INTF *') is disallowed with ARC}}
#endif
}
```


https://reviews.llvm.org/D31005