<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><br class=""><div><blockquote type="cite" class=""><div class="">On May 24, 2016, at 11:42 AM, Bob Wilson <<a href="mailto:bob.wilson@apple.com" class="">bob.wilson@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><blockquote type="cite" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><br class="Apple-interchange-newline">On May 24, 2016, at 11:21 AM, Manman <<a href="mailto:mren@apple.com" class="">mren@apple.com</a>> wrote:<br class=""><br class=""><br class=""><blockquote type="cite" class="">On May 23, 2016, at 8:15 PM, Bob Wilson <<a href="mailto:bob.wilson@apple.com" class="">bob.wilson@apple.com</a>> wrote:<br class=""><br class="">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.<br class=""></blockquote><br class="">The approach looks good to me in general.<span class="Apple-converted-space"> </span><br class=""><blockquote type="cite" class="">diff --git lib/Sema/AnalysisBasedWarnings.cpp lib/Sema/AnalysisBasedWarnings.cpp<br class="">index 3f2c41b..eb45315 100644<br class="">--- lib/Sema/AnalysisBasedWarnings.cpp<br class="">+++ lib/Sema/AnalysisBasedWarnings.cpp<br class="">@@ -1334,6 +1334,12 @@ static void diagnoseRepeatedUseOfWeak(Sema &S,<br class=""> else<br class=""> llvm_unreachable("Unexpected weak object kind!");<br class=""><br class="">+ // Do not warn about IBOutlet weak property receivers being set to null<br class="">+ // since they are typically only used from the main thread.<br class="">+ if (const ObjCPropertyDecl *Prop = dyn_cast<ObjCPropertyDecl>(D))<br class="">+ if (Prop->hasAttr<IBOutletAttr>())<br class="">+ continue;<br class="">+<br class=""> // Show the first time the object was read.<br class=""> S.Diag(FirstRead->getLocStart(), DiagKind)<br class=""> << int(ObjectKind) << D << int(FunctionKind)<br class=""></blockquote><br class="">Should we guard the call to diagnoseRepeatedUseOfWeak, instead of checking the decl inside a loop in diagnoseRepeatedUseOfWeak?<br class=""><br class="">if (S.getLangOpts().ObjCWeak &&<br class=""> !Diags.isIgnored(diag::warn_arc_repeated_use_of_weak, D->getLocStart()))<br class=""> diagnoseRepeatedUseOfWeak(S, fscope, D, AC.getParentMap());<br class="">—> check IBOutlet here?<br class=""></blockquote><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">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.</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">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:</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class=""> const NamedDecl *D = Key.getProperty();</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">We should rename that to avoid potential confusion. I can do that as a follow-up change.</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""></div></blockquote><div><br class=""></div>Yes, I missed the local variable “D”.</div><div><br class=""><blockquote type="cite" class=""><div class=""><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><blockquote type="cite" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><br class=""><blockquote type="cite" class="">diff --git lib/Sema/SemaPseudoObject.cpp lib/Sema/SemaPseudoObject.cpp<br class="">index 62c823b..c93d800 100644<br class="">--- lib/Sema/SemaPseudoObject.cpp<br class="">+++ lib/Sema/SemaPseudoObject.cpp<br class="">@@ -578,7 +578,7 @@ bool ObjCPropertyOpBuilder::isWeakProperty() const {<br class=""> if (RefExpr->isExplicitProperty()) {<br class=""> const ObjCPropertyDecl *Prop = RefExpr->getExplicitProperty();<br class=""> if (Prop->getPropertyAttributes() & ObjCPropertyDecl::OBJC_PR_weak)<br class="">- return !Prop->hasAttr<IBOutletAttr>();<br class="">+ return true;<br class=""><br class=""> T = Prop->getType();<br class=""> } else if (Getter) {<br class=""></blockquote><br class="">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?<br class=""></blockquote><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">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.</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""></div></blockquote><div><br class=""></div>Thanks for the explanation!</div><div><br class=""></div><div>Patch looks good to me,</div><div>Manman</div><div><br class=""><blockquote type="cite" class=""><div class=""><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><blockquote type="cite" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><br class="">Thanks,<br class="">Manman<br class=""><br class=""><blockquote type="cite" class=""><a href="rdar://problem/21366461" class="">rdar://problem/21366461</a><br class=""><br class=""><clang.patch></blockquote></blockquote></div></blockquote></div><br class=""></body></html>