> On May 24, 2016, at 11:42 AM, Bob Wilson <bob.wil...@apple.com> wrote:
>
>>
>> On May 24, 2016, at 11:21 AM, Manman <m...@apple.com> wrote:
>>
>>
>>> On May 23, 2016, at 8:15 PM, Bob Wilson <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,
>> 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
