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

[Manman via cfe-commits]

> 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?

> 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?

Thanks,
Manman

> rdar://problem/21366461
> 
> <clang.patch>