> On May 24, 2016, at 11:46 AM, Manman Ren <m...@apple.com> wrote: > >> >> On May 24, 2016, at 11:42 AM, Bob Wilson <bob.wil...@apple.com >> <mailto:bob.wil...@apple.com>> wrote: >> >>> >>> On May 24, 2016, at 11:21 AM, Manman <m...@apple.com >>> <mailto:m...@apple.com>> wrote: >>> >>> >>>> On May 23, 2016, at 8:15 PM, Bob Wilson <bob.wil...@apple.com >>>> <mailto:bob.wil...@apple.com>> wrote: >>>> >>>> Revision r211132 was supposed to disable -Warc-repeated-use-of-weak for >>>> Objective-C properties marked with the IBOutlet attribute. Those >>>> properties are supposed to be weak but they are only accessed from the >>>> main thread so there is no risk of asynchronous updates setting them to >>>> nil. That combination makes -Warc-repeated-use-of-weak very noisy. The >>>> previous change only handled one kind of access to weak IBOutlet >>>> properties. Instead of trying to add checks for all the different kinds of >>>> property accesses, this patch removes the previous special case check and >>>> adds a check at the point where the diagnostic is reported. >>> >>> The approach looks good to me in general. >>>> diff --git lib/Sema/AnalysisBasedWarnings.cpp >>>> lib/Sema/AnalysisBasedWarnings.cpp >>>> index 3f2c41b..eb45315 100644 >>>> --- lib/Sema/AnalysisBasedWarnings.cpp >>>> +++ lib/Sema/AnalysisBasedWarnings.cpp >>>> @@ -1334,6 +1334,12 @@ static void diagnoseRepeatedUseOfWeak(Sema &S, >>>> else >>>> llvm_unreachable("Unexpected weak object kind!"); >>>> >>>> + // Do not warn about IBOutlet weak property receivers being set to >>>> null >>>> + // since they are typically only used from the main thread. >>>> + if (const ObjCPropertyDecl *Prop = dyn_cast<ObjCPropertyDecl>(D)) >>>> + if (Prop->hasAttr<IBOutletAttr>()) >>>> + continue; >>>> + >>>> // Show the first time the object was read. >>>> S.Diag(FirstRead->getLocStart(), DiagKind) >>>> << int(ObjectKind) << D << int(FunctionKind) >>> >>> Should we guard the call to diagnoseRepeatedUseOfWeak, instead of checking >>> the decl inside a loop in diagnoseRepeatedUseOfWeak? >>> >>> if (S.getLangOpts().ObjCWeak && >>> !Diags.isIgnored(diag::warn_arc_repeated_use_of_weak, D->getLocStart())) >>> diagnoseRepeatedUseOfWeak(S, fscope, D, AC.getParentMap()); >>> —> check IBOutlet here? >> >> diagnoseRepeatedUseOfWeak is used to report all of the issues within a >> function. Some of those may involve IBOutlet properties and others not. We >> have to check each one separately. >> >> Your comment prompted me to look more closely and I realized that the code >> is confusing because there is a local variable “D” that shadows the “const >> Decl *D” argument of diagnoseRepeatedUseOfWeak: >> >> const NamedDecl *D = Key.getProperty(); >> >> We should rename that to avoid potential confusion. I can do that as a >> follow-up change. > > Yes, I missed the local variable “D”. > >> >>> >>>> diff --git lib/Sema/SemaPseudoObject.cpp lib/Sema/SemaPseudoObject.cpp >>>> index 62c823b..c93d800 100644 >>>> --- lib/Sema/SemaPseudoObject.cpp >>>> +++ lib/Sema/SemaPseudoObject.cpp >>>> @@ -578,7 +578,7 @@ bool ObjCPropertyOpBuilder::isWeakProperty() const { >>>> if (RefExpr->isExplicitProperty()) { >>>> const ObjCPropertyDecl *Prop = RefExpr->getExplicitProperty(); >>>> if (Prop->getPropertyAttributes() & ObjCPropertyDecl::OBJC_PR_weak) >>>> - return !Prop->hasAttr<IBOutletAttr>(); >>>> + return true; >>>> >>>> T = Prop->getType(); >>>> } else if (Getter) { >>> >>> I wonder if this change is necessary to make the testing case pass, or is >>> it introduced for clarity, to better reflect the function name >>> isWeakProperty? >> >> This is the one special-case check that was introduced by r211132. I removed >> it because there is no need for it after we add the check in >> diagnoseRepeatedUseOfWeak. > > Thanks for the explanation! > > Patch looks good to me, > Manman
Thanks for the review. Patch is in r270665. I renamed the variable to fix the parameter shadowing in r270666. > >> >>> >>> Thanks, >>> Manman >>> >>>> rdar://problem/21366461 <rdar://problem/21366461> >>>> >>>> <clang.patch>
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
