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

[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 &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.
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