> 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.
>
>> 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,
> Manman
>
>> rdar://problem/21366461
>>
>> <clang.patch>
>
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
