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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
