<html><head><meta http-equiv="Content-Type" content="text/html charset=us-ascii"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div>On 22 Oct 2013, at 18:43, Jordan Rose <<a href="mailto:jordan_rose@apple.com">jordan_rose@apple.com</a>> wrote:</div><div><br><blockquote type="cite"><meta http-equiv="Content-Type" content="text/html charset=us-ascii"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div>Looking good! I do still have a few reservations, though. The main one is this:</div><div><br></div><div><div>+ if (const SymIntExpr *SIExpr = dyn_cast<SymIntExpr>(*I)) {</div><div>+ // A symbol is being compared to an int value. The Assumption value</div><div>+ // is adjusted so that it is checking the symbol exists.</div><div>+ BinaryOperatorKind op = SIExpr->getOpcode();</div><div>+ bool rhs = SIExpr->getRHS() != 0;</div><div>+ if ((op == BO_EQ && rhs == false) || (op == BO_NE && rhs == true))</div><div>+ Assumption = !Assumption;</div><div>+</div><div>+ continue;</div><div>+ }</div><div><br></div><div>I'm not actually sure which case this handles...is it after a weak function pointer has been converted to bool? Because it also matches this case:</div><div><br></div><div>NSAppKitVersionNumber > NSAppKitVersionNumber10_7</div><div><br></div><div>At the very least, you should be checking that the opcode is one of the operators you expect.</div><div><br></div></div></div></blockquote><div><br></div><div>Oops, yes, that was careless. This part of the code handles situations like:</div><div><br></div><div><div style="margin: 0px; font-size: 14px; font-family: Monaco; "><div style="margin: 0px; "><span style="color: #bb2ca2">if</span> ([obj respondsToSelector:<span style="color: #bb2ca2">@selector</span>(doVersion20Stuff)] == <span style="color: #bb2ca2">YES</span>) {</div><div style="margin: 0px; color: rgb(0, 132, 0); "><span style="color: #000000"> </span>// do version 20 stuff</div><div style="margin: 0px; ">}</div></div></div><div><br></div><div>If the Assumption bool was not matched to the boolean operator, then the following case would also set version check status:</div><div><br></div><div><div style="font-size: 14px; margin: 0px; font-family: Monaco; "><div style="margin: 0px; "><span style="color: rgb(187, 44, 162); ">if</span> ([obj respondsToSelector:<span style="color: rgb(187, 44, 162); ">@selector</span>(doVersion20Stuff)] == <font color="#bb2ca2">NO</font>) {</div><div style="margin: 0px; color: rgb(0, 132, 0); "><span style="color: rgb(0, 0, 0); "> </span>// don't do version 20 stuff</div><div style="margin: 0px; ">}</div></div></div><br><blockquote type="cite"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div><div>Besides that, here are a storm of little notes:</div><div><br></div><div><div>+ if (ExplodedNode *N = C.generateSink(StNull, 0, &Tag)) {</div><div>+ ImplicitNullDerefEvent Event = { L, false, N, &C.getBugReporter() };</div><div>+ dispatchEvent(Event);</div><div>+ }</div></div><div><br></div><div>Extra indentation here.</div><div><br></div><div><div><br></div><div>+ if (!(isa<SimpleCall>(&Call) || isa<ObjCMethodCall>(&Call)))</div></div><div><br></div><div>isa<> should work on references as well as pointers.</div><div><br></div><div><br></div><div><div>+ // If we have an objc method, check for the classes introduced version.</div><div>+ if (const ObjCMethodDecl *MD = dyn_cast<ObjCMethodDecl>(D)) {</div><div>+ VersionTuple ClassIntroduced;</div><div>+ const DeclContext *DC = MD->getDeclContext();</div><div>+ if (const ObjCProtocolDecl *PD = dyn_cast<ObjCProtocolDecl>(DC))</div><div>+ ClassIntroduced = versionIntroduced(PD);</div><div>+ else</div><div>+ ClassIntroduced = versionIntroduced(MD->getClassInterface());</div><div>+</div><div>+ Introduced = std::max(Introduced, ClassIntroduced);</div><div>+ }</div></div><div><br></div><div>I think you only need to do this if the method itself doesn't have an introduced version...it's probably safe to assume that headers are sane and won't have a method introduced before the class that contains it.</div><div><br></div><div>Also, typos/formatting: "ObjC" (or full "Objective-C"), and "class's".</div><div><br></div><div>Nice handling of protocols/categories/implementation/interface.</div><div><br></div><div><br></div><div>+ // Walk through the symbols children looking for a version check.</div><div><br></div><div>Typo: "symbol's"</div><div><br></div><div><br></div><div><div>+ // If this is a weak function, it is not OK to derefence if availability</div><div>+ // has not been checked for.</div></div><div><br></div><div>Typo: "dereference"</div><div><br></div><div><br></div><div><div>+ os << "The method is introduced in ";</div><div>+ os << Introduced;</div><div>+ os << ", later than the deployment target ";</div><div>+ os << deploymentTarget(State);</div></div><div><div>+</div><div>+ BugReport *R = new BugReport(*BT, os.str(), N);</div><div>+ BR->emitReport(R);</div></div><div><br></div><div>I'm not so happy with the message. For one thing, this applies to functions as well as methods, and may eventually apply to enumerations, their enumerators, and/or global variables. For another, it doesn't quite say what's wrong. How about <i>"Calling</i> method introduced in X.Y (deployment target is X.W)"?</div><div><br></div><div>It's also too bad that there's no highlight range here, but I guess the location is accurate enough.</div><div><br></div><div><br></div></div></div></blockquote><div><br></div><div>Yes, I did originally have the highlight range, but I could not work out a way to get this from the ImplicitNullDerefEvent, so I removed it.</div><br><blockquote type="cite"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div><div>+const Decl* UnavailableMethodChecker::weakDeclForSymbol(SymbolRef Sym) const {</div><div><br></div><div>Asterisk on the right, please. </div><div><br></div><div><br></div><div><div>+const Decl*</div><div>+UnavailableMethodChecker::weakDeclForRegion(const MemRegion *R) const {</div></div><div><br></div><div>...and here too.</div><div><br></div><div><br></div><div><div>+ }</div><div>+</div><div>+ else if (const SymbolConjured *SConj = dyn_cast<SymbolConjured>(*I)) {</div></div><div><br></div><div>Please keep these on one line. If it looks too crowded, you can add a blank line before the close brace.</div><div><br></div><div><div><br></div><div>+ // This is an objective C message, first get the class availability.</div></div><div><br></div><div>Formatting: "Objective-C"</div><div><br></div><div><div><br></div><div>+ std::string SelStr = ME->getSelector().getAsString();</div><div>+ if (SelStr == "respondsToSelector:" ||</div><div>+ SelStr == "instancesRespondToSelector:") {</div></div><div><br></div><div>getAsString() is expensive because it constructs a new string on the heap. Better to check that it is a unary selector, then grab the first selector piece and check that.</div><div><br></div></div></div></blockquote><div><br></div><div>Not sure I get you here, isn't only 'class' a unary selector? I changed this to breaking early if there is more than 1 arg, and using the selector name for the first slot only.</div><br><blockquote type="cite"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div><div><br></div><div>+ const ObjCSelectorExpr *SelExpr = cast<ObjCSelectorExpr>(ME->getArg(0));</div><div><br></div><div>This could fail if the user passes a SEL variable.</div><div><br></div></div></div></blockquote><div><br></div><div>Well spotted. I have added a helper method to extract the ObjCSelectorExpr in the case of a SEL variable, it is very ugly code though, is there an easier way to do this? see selectorForArgument().</div><br><blockquote type="cite"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div><div><br></div><div><div>+ // We are only checking for SymInt and Symbol comparisons</div><div>+ else</div><div>+ break;</div></div><div><br></div><div>I'm not sure you want to break...perhaps just ignore these?</div><div><br></div></div></div></blockquote><div><br></div><div>I suppose I was simply trying to restrict the version checks as much as possible. I could not think of a valid case that would require walking further through the symbols children. I have removed it anyway, as I also cannot think of a case it helps.</div><br><blockquote type="cite"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div><div><br></div><div>Thanks for working on this; sorry for drawing it out even more!</div><div>Jordan</div></div></div></blockquote></div><div><br></div><div>So, latest patch attached, I hope I got all the aesthetic things sorted.</div><div>Richard</div><div><br></div><div></div></body></html>