<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:46 AM, Manman Ren <<a href="mailto:mren@apple.com" class="">mren@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div 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" class=""><div class=""><br class="Apple-interchange-newline">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" class="" 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;"><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 class="" 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;"><span class="" 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;">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 class="" 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;"><br class="" 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;"><span class="" 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;">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 class="" 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;"><br class="" 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;"><span class="" 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;"> const NamedDecl *D = Key.getProperty();</span><br class="" 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;"><br class="" 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;"><span class="" 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;">We should rename that to avoid potential confusion. I can do that as a follow-up change.</span><br class="" 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;"></div></blockquote><div class=""><br class=""></div>Yes, I missed the local variable “D”.</div><div 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=""><div class=""><br class="" 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;"><blockquote type="cite" class="" 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;"><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 class="" 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;"><span class="" 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;">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 class="" 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;"></div></blockquote><div class=""><br class=""></div>Thanks for the explanation!</div><div 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=""></div><div 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="">Patch looks good to me,</div><div 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="">Manman</div></div></blockquote><div><br class=""></div>Thanks for the review.</div><div>Patch is in r270665.</div><div>I renamed the variable to fix the parameter shadowing in r270666.</div><div><br class=""></div><div><blockquote type="cite" class=""><div class=""><div 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=""><div class=""><br class="" 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;"><blockquote type="cite" class="" 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;"><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></div></blockquote></div><br class=""></body></html>