<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; "><br><div><div>On Oct 24, 2013, at 1:19 , Richard <<a href="mailto:tarka.t.otter@gmail.com">tarka.t.otter@gmail.com</a>> wrote:</div><br class="Apple-interchange-newline"><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>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>It's also too bad that there's no highlight range here, but I guess the location is accurate enough.</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></div></div></blockquote><div><br></div><div>The ugly way to do it would be to crawl into the sink node's ProgramPoint, but I'm happy to add a Stmt* to the ImplicitNullDerefEvent that's allowed to be null. That can be in a later patch, though.</div><div><br></div><br><blockquote type="cite"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div><blockquote type="cite"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><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></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></div></div></blockquote><div><br></div><div>It looks like we're inconsistent about whether "class" is "unary" or "nullary"; the factory methods say "nullary", with "unary" being single-argument, but the predicate methods say "unary" means "non-keyword", i.e. "no arguments". I think the factory methods are right. Anyway, yes, your new code is what I meant.</div><div><br></div><br><blockquote type="cite"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div><blockquote type="cite"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><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></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></div></div></blockquote><div><br></div><div>The way you've done it will break for re-assignments, so that won't work. I think the right thing to do is just ignore that case for now; in the long term, I think the correct solution is to model SEL regions in the same sort of way as we do string literals.</div><div><br></div><div><br></div><div>I'm still concerned about the diagnostic text:</div><div><br></div><div><div>+ os << "Calling method introduced in ";</div><div>+ os << Introduced;</div><div>+ os << " (deployment target is ";</div><div>+ os << deploymentTarget(State);</div><div>+ os << ")";</div><div><br></div><div>At the very least we need to distinguish "method" and "function"; for bonus points, saying that the entire class or protocol was introduced in version X would be nice polish.</div><div><br></div><div>Jordan</div></div></div></body></html>