[PATCH] arc-repeated-use-of-weak should not warn about IBOutlet properties

[Bob Wilson via cfe-commits]

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