<html><head><meta http-equiv="Content-Type" content="text/html charset=windows-1252"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><div>On Nov 6, 2013, at 7:18 AM, Tarka T <<a href="mailto:tarka.t.otter@gmail.com">tarka.t.otter@gmail.com</a>> wrote:</div><div><br class="Apple-interchange-newline"><blockquote type="cite"><div class="gmail_extra" style="font-family: Helvetica; font-size: 14px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: 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;">OK, I think the current patch covers everything then. Let me know if you see any more issues.</div><div class="gmail_extra" style="font-family: Helvetica; font-size: 14px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: 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></div><div class="gmail_extra" style="font-family: Helvetica; font-size: 14px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: 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;">Regards,</div><div class="gmail_extra" style="font-family: Helvetica; font-size: 14px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: 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;">Richard</div></blockquote><br></div><div>Hi Richard,</div><div><br></div><div>First, thank you so much for working on this. I’m sorry I haven’t had a chance to comment sooner. My comments are a mixture of nitpicky (which are minor, but probably worth addressing) as well as a few high-level comments. Overall I think this is a great foundation for further work on what I think could be really valuable checker.</div><div><br></div><div>My comments will be roughly chronologically thought UnavailableChecker.cpp, with a few broader comments interlaced. My first high-level comment is that this is a very algorithmic checker; it’s doing something quite interesting. It would be great to see more liberal use of comments explaining the motivation behind various pieces. It allows someone to better understand what the checker is doing, but also allows one (including myself) to better understand if the checker is doing quite what was intended. I’ll provide specific comments throughout the file on where I think more information is needed.</div><div><br></div><div><blockquote type="cite">#include "ClangSACheckers.h"<br>#include "clang/AST/Attr.h"<br>#include "clang/AST/DeclObjC.h"<br>#include "clang/Basic/TargetInfo.h"<br>#include "clang/StaticAnalyzer/Core/Checker.h"<br>#include "clang/StaticAnalyzer/Core/CheckerManager.h"<br>#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"<br>#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"<br>#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"<br>#include "llvm/ADT/SmallString.h”<br><br></blockquote><div><br></div><div>To start off, this file is missing the standard LLVM boilerplate, which mentions the name of the file and what its purpose is. In this case, a brief description of the purpose of the checker seems warranted.</div><br><br><blockquote type="cite">using namespace clang;<br>using namespace ento;<br><br>namespace {<br> class UnavailableMethodChecker<br> : public Checker< check::PreCall,<br> eval::Assume,<br> check::Event<ImplicitNullDerefEvent> > {<br> mutable OwningPtr<BugType> BT;<br> public:<br> void checkPreCall(const CallEvent &Call, CheckerContext &Ctx) const;<br> ProgramStateRef evalAssume(ProgramStateRef State, SVal Cond,<br> bool Assumption) const;<br> void printState(raw_ostream &Out, ProgramStateRef State,<br> const char *NL, const char *Sep) const;<br> void checkEvent(ImplicitNullDerefEvent Event) const;<br></blockquote><div><br></div><div>These are standard API hooks in the Checker API, but it would be great to have some doxygen comments on these.</div><br><blockquote type="cite"><br> private:<br></blockquote><div><br></div><div>These next methods are internal to the checker, and they really would be well-served with some doxygen comments. When reviewing the checker, I had to reverse engineer every piece. Having some high-level comments here that explain what these methods do would be really helpful.</div><br><blockquote type="cite"> void displayError(const ProgramStateRef State, VersionTuple Introduced,<br> const NamedDecl *D, ExplodedNode *N,<br> BugReporter *BR) const;<br> const Decl *weakDeclForSymbol(SymbolRef Sym) const;<br> const Decl *weakDeclForRegion(const MemRegion *MR) const;<br></blockquote><div><br></div><div>I’d prefer these to be named “getWeakDeclForSymbol” instead of “weakDeclForSymbol”. This follows the LLVM naming conventions where functions should be verb phrases:</div><div><br></div><div> "Function names should be verb phrases (as they represent actions), and command-like function should be imperative. The name should be camel case, and start with a lower case letter (e.g. openFile() or isFoo()).”</div><div><br></div><div>For more information, see:</div><div><br></div><div> <a href="http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly">http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly</a></div><div><br></div><div>In “weakDeclForSymbol” there is no verb; “for” is a preposition. I know this comment may seem a bit silly, and while I respect your style of names we should aim to keep them consistent with the rest of the codebase.</div><br><blockquote type="cite"> VersionTuple deploymentTarget(ProgramStateRef State) const;<br></blockquote><div><br></div><div>I don’t think this method is even needed. More comments below.</div><br><blockquote type="cite"> VersionTuple versionIntroduced(const Decl *D) const;<br> VersionTuple versionCheckForSymbol(SymbolRef S, bool Assumption) const;<br></blockquote><div><br></div><div>Same comment as above: “getVersionIntroduced”, etc. These also really need doxygen comments.</div><br><blockquote type="cite"> VersionTuple getCheckedForVersion(ProgramStateRef State) const;<br> ProgramStateRef setCheckedForVersion(ProgramStateRef State,<br> VersionTuple V) const;<br> };<br></blockquote><blockquote type="cite">}<br><br>REGISTER_TRAIT_WITH_PROGRAMSTATE(CheckedVersionMajor, unsigned)<br>REGISTER_TRAIT_WITH_PROGRAMSTATE(CheckedVersionMinor, unsigned)<br>REGISTER_TRAIT_WITH_PROGRAMSTATE(CheckedVersionSubminor, unsigned)<br></blockquote><div><br></div><div>Having three maps is far less efficient than having one map for VersionTuple. It’s not just the size of the individual pieces of data; its the metadata to represent the map itself. All of these seemed to be changed together in lockstep, it is is far more efficient to both retrieve and set data from one map instead of 3. To understand the efficiency, each map is represented by a functional AVL binary tree. Having three trees requires balancing and querying three trees, as well as the memory associated with three trees.</div><br><blockquote type="cite"><br>void UnavailableMethodChecker::checkPreCall(const CallEvent &Call,<br> CheckerContext &Ctx) const {<br> // Bail early if there is no deployment target.<br> ProgramStateRef State = Ctx.getState();<br> VersionTuple TargetMinVersion = deploymentTarget(State);<br> if (TargetMinVersion.empty())<br> return;<br></blockquote><div><br></div><div>Since this check ultimately only relies on ASTContext, I think this check can be hoisted into registerUnavailableMethodChecker(). That way the checker doesn’t even get run if this information is not available. It then allows you to remove ‘deploymentTarget()’ entirely, and not call it.</div><br><blockquote type="cite"><br> if (!(isa<SimpleCall>(Call) || isa<ObjCMethodCall>(Call)))<br> return;<br><br> if (isa<ObjCMethodCall>(Call)) {<br> const ObjCMethodCall &ObjCCall = cast<ObjCMethodCall>(Call);<br></blockquote><div><br></div><div>This should be a dyn_cast, not an isa plus a cast. For example:</div><div><br></div><div> if (const ObjCMethodCal *ObjCCall = dyn_cast<ObjCMethodCall>(&Call))</div><br><blockquote type="cite"><br> // ObjC methods with no type information about the receiver cannot be<br> // reasoned about accurately.<br> const ObjCMessageExpr *ME = ObjCCall.getOriginExpr();<br> if (!ME->getReceiverInterface())<br> return;<br><br> // Array and dictionary subscripts can be ignored, they are implemented<br> // for lower deployment targets by the runtime.<br> if (ObjCCall.getMessageKind() == OCM_Subscript)<br> return;<br> <br> // If this method is calling self of super with the same selector, then we<br> // can ignore any version checking.<br> if (ObjCCall.isReceiverSelfOrSuper()) {<br> const StackFrameContext *SFC = Ctx.getStackFrame();<br> if (const ObjCMethodDecl *D = dyn_cast<ObjCMethodDecl>(SFC->getDecl())) {<br> if (D->getSelector() == ME->getSelector())<br> return;<br> }<br> }<br> }<br><br> const Decl *D = Call.getDecl();<br> if (!D)<br> return;<br><br> VersionTuple Introduced = versionIntroduced(D);<br><br> // If we have an ObjC method with no introduced version, check the class's or<br> // the protocol's introduced version.<br> const NamedDecl *VersionDecl = cast<NamedDecl>(D);<br> if (Introduced.empty()) {<br> if (const ObjCMethodDecl *MD = dyn_cast<ObjCMethodDecl>(D)) {<br> const DeclContext *DC = MD->getDeclContext();<br> if (const ObjCProtocolDecl *PD = dyn_cast<ObjCProtocolDecl>(DC)) {<br> Introduced = versionIntroduced(PD);<br> VersionDecl = PD;<br> } else {<br> const ObjCInterfaceDecl *ID = MD->getClassInterface();<br> Introduced = versionIntroduced(ID);<br> VersionDecl = ID;<br> }<br></blockquote><blockquote type="cite"> }<br> }<br></blockquote><div><br></div><div>This looks like this can be hoisted directly into “versionIntroduced” (or rather, “getVersionIntroduced”). That way getVersionIntroduced() computes the truth as a self-encapsulated information, regardless of whether it consults the map or has to compute some of the information lazily. Also, it doesn’t look like all of the callers to getVersionIntroduced() compute this information consistently; hoisting it into getVersionIntroduced() would solve that problem.<br></div><br><blockquote type="cite"><br> if (TargetMinVersion >= Introduced)<br> return;<br><br> // We have a method being called that is available later than the minimum<br> // deployment target, so we need to check for any availability checks.<br> VersionTuple CheckedVersion = getCheckedForVersion(State);<br> if (CheckedVersion >= Introduced)<br> return;<br><br> if (ExplodedNode *N = Ctx.addTransition(State)) {<br> displayError(State, Introduced, VersionDecl, N, &Ctx.getBugReporter());<br> }<br></blockquote><div><br></div><div>This is good, but maybe we should only emit a warning once along a path. I’m concerned about a cascade of warnings. For example, say I had 10 function calls, one after the other, that were used unsafely, but would be safely guarded with a single check. We really don’t need 10 warnings; we only need one. If we stopped tracking version information along a path, we could perhaps only emit the warning once? One way to do that would be to pump up the checked version to something really high and store that in the state. A call to ‘addTransition()’ doesn’t stop the path; the analyzer will keep exploring a path.</div><div><br></div><blockquote type="cite">}<br><br>ProgramStateRef UnavailableMethodChecker::evalAssume(ProgramStateRef State,<br> SVal Cond,<br> bool Assumption) const {<br> // Bail early if there is no deployment target set.<br> VersionTuple TargetMinVersion = deploymentTarget(State);<br> if (TargetMinVersion.empty())<br> return State;<br></blockquote><div><br></div><div>I believe this can be removed if we do this check during checker registration.</div><br><blockquote type="cite"><br> SymbolRef S = Cond.getAsSymbol();<br> if (!S)<br> return State;<br><br> // Walk through the symbol's children looking for a version check.<br> VersionTuple V = versionCheckForSymbol(S, Assumption);<br> return setCheckedForVersion(State, V);<br>}<br><br>void UnavailableMethodChecker::checkEvent(ImplicitNullDerefEvent Event) const {<br> if (Event.IsLoad)<br> return;<br><br> // Bail early if there is no deployment target.<br> ProgramStateRef State = Event.SinkNode->getState();<br> VersionTuple TargetMinVersion = deploymentTarget(State);<br> if (TargetMinVersion.empty())<br> return;<br></blockquote><div><br></div>I believe this can be removed if we do this check during checker registration.</div><div><br><blockquote type="cite"><br> const MemRegion *MR = Event.Location.getAsRegion();<br> if (!MR)<br> return;<br><br> // If this is a weak function, it is not OK to dereference if availability<br> // has not been checked for.<br> if (const Decl *D = weakDeclForRegion(MR)) {<br> VersionTuple Introduced = versionIntroduced(D);<br> VersionTuple Checked = getCheckedForVersion(State);<br> if (Introduced > Checked)<br> // TODO: get the highlight range for the event.<br> displayError(State, Introduced, cast<NamedDecl>(D),<br> Event.SinkNode, Event.BR);<br> }<br>}<br></blockquote><div><br></div><div>Looks good.</div><br><blockquote type="cite"><br>void UnavailableMethodChecker::printState(raw_ostream &Out,<br> ProgramStateRef State,<br> const char *NL,<br> const char *Sep) const {<br> VersionTuple CheckedVersion = getCheckedForVersion(State);<br> if (!CheckedVersion.empty()) {<br> Out << Sep;<br> Out << "Checked availability: " << CheckedVersion.getAsString();<br> }<br>}<br><br>void UnavailableMethodChecker::displayError(const ProgramStateRef State,<br> VersionTuple Introduced,<br> const NamedDecl *D,<br> ExplodedNode *N,<br> BugReporter *BR) const {<br> if (!BT)<br> BT.reset(new BugType("API Unavailable", "API Misuse"));<br><br> SmallString<128> sbuf;<br> llvm::raw_svector_ostream os(sbuf);<br> switch (D->getKind()) {<br> case Decl::Function:<br> os << "Calling function";<br> break;<br><br> case Decl::ObjCMethod:<br> os << "Calling method";<br> break;<br><br> case Decl::ObjCProtocol:<br> os << "Calling method belonging to protocol ";<br> os << "'" << *D << "’";<br></blockquote><div><br></div><div>Minor, just include ‘*D’ on the previous line:</div><div><br></div><div> os << “Calling method belonging to protocol '“ << *D << “‘";</div><div><br></div><div>You have plenty of space.</div><br><blockquote type="cite"> break;<br><br> case Decl::ObjCInterface:<br> os << "Calling method belonging to class ";<br> os << "'" << *D << "'";<br> break;<br></blockquote><div><br></div>Same comment.</div><div><br><blockquote type="cite"><br> default:<br> llvm_unreachable("Unknown Decl kind.");<br> }<br><br> ASTContext &Ctx = State->getStateManager().getContext();<br> StringRef TargetPlatform = Ctx.getTargetInfo().getPlatformName();<br> StringRef PrettyPlatformName =<br> AvailabilityAttr::getPrettyPlatformName(TargetPlatform);<br> if (PrettyPlatformName.empty())<br> PrettyPlatformName = TargetPlatform;<br><br> os << " introduced in " << PrettyPlatformName << " " << Introduced;<br> os << " (deployment target is " << deploymentTarget(State) << ")";<br> BugReport *R = new BugReport(*BT, os.str(), N);<br> BR->emitReport(R);<br>}</blockquote><div><br></div><div>Looks good.</div><br><blockquote type="cite"><br>VersionTuple<br>UnavailableMethodChecker::deploymentTarget(ProgramStateRef State) const {<br> ProgramStateManager &PSM = State->getStateManager();<br> return PSM.getContext().getTargetInfo().getPlatformMinVersion();<br>}<br></blockquote><div><br></div><div>I believe that this method can just be removed. We can query the deployment target at checker construction time, and save the VersionTuple with the checker object itself.</div><br><blockquote type="cite"><br>VersionTuple UnavailableMethodChecker::versionIntroduced(const Decl *D) const {<br> if (D)<br></blockquote><div><br></div><div>This null check is unnecessary. All calls to versionIntroduced() are dominated by a null check. While this is defensive programming, it makes it looks like the contact of the function is weaker than it really is.</div><br><blockquote type="cite"> if (const AvailabilityAttr *Availability = D->getAttr<AvailabilityAttr>())<br> return Availability->getIntroduced();<br><br> return VersionTuple();<br>}<br><br>const Decl *UnavailableMethodChecker::weakDeclForSymbol(SymbolRef Sym) const {<br> const SymbolExtent *SE = dyn_cast<SymbolExtent>(Sym);<br> if (!SE)<br> return NULL;<br><br> return weakDeclForRegion(SE->getRegion());<br></blockquote><div><br></div><div>Minor, but this can be written more concisely as:</div><div><br></div><div> if (const SymbolExtent *SE = dyn_cast<SymbolExtent>(Sym))</div><div> return getWeakDeclForRegion(SE->getRegion());</div><div> return NULL;</div><div><br></div><div>That’s 3 lines instead of 5. Again it’s really minor; nothing really wrong with it.</div><div><br></div><blockquote type="cite">}<br><br>const Decl<br>*UnavailableMethodChecker::weakDeclForRegion(const MemRegion *R) const {<br> if (!isa<FunctionTextRegion>(R))<br> return NULL;<br><br> const FunctionTextRegion *FTR = cast<FunctionTextRegion>(R);<br> const FunctionDecl *D = cast<FunctionDecl>(FTR->getDecl());<br> return D->isWeak() ? D : NULL;<br>}<br></blockquote><div><br></div><div>Looks good.</div><br><blockquote type="cite"><br>VersionTuple<br>UnavailableMethodChecker::versionCheckForSymbol(SymbolRef Sym,<br> bool Assumption) const {<br> // Iterate over the expressions symbols looking for version checks.<br> for (SymExpr::symbol_iterator I = Sym->symbol_begin(), E = Sym->symbol_end();<br> I != E; ++I) {<br></blockquote><div><br></div><div>The follow logic is basically “pattern matching” over symbolic expressions. I sometimes think it is helpful to hard a brief comment over such logic to illustrate the idiom being matched, so it is clear what is covered and what isn’t. It also serves as the inspiration for test cases.</div><br><blockquote type="cite"> if (const SymIntExpr *SIExpr = dyn_cast<SymIntExpr>(*I)) {<br> // A symbol is being compared to an int value. The Assumption value<br> // is adjusted so that it is checking the symbol exists.<br> BinaryOperatorKind op = SIExpr->getOpcode();<br> if (op != BO_EQ && op != BO_NE)<br> break;<br><br> bool rhs = SIExpr->getRHS() != 0;<br></blockquote><div><br></div><div>Please follow LLVM naming conventions consistently when it comes to variable names. In this case, please keep ‘rhs’ as ‘RHS’:</div><div><br></div><div> "Variable names should be nouns (as they represent state). The name should be camel case, and start with an upper case letter (e.g. Leader or Boats)."</div><div><div><br></div><div>I know the analyzer doesn’t consistently follow this style; we’re trying to gradually fix that.</div><br></div><br><blockquote type="cite"> if ((op == BO_EQ && rhs == false) || (op == BO_NE && rhs == true))<br> Assumption = !Assumption;<br></blockquote><div><br></div><div>Minor:</div><div><br></div><div> rhs == false can be: !RHS</div><div> rhs == true can be: RHS</div><br><blockquote type="cite"><br> continue;<br> }<br><br> // We are only interested in checks for existence.<br></blockquote><div><br></div><div>This comment is a bit too terse. “existence” of what? It’s missing the subject of the sentence.</div><br><blockquote type="cite"> if (!Assumption)<br> break;<br></blockquote><blockquote type="cite"><br></blockquote><div><br></div><div>Please also include a comment here. Why is the SymbolExtent being checked here?</div><br><blockquote type="cite"> if (const SymbolExtent *SE = dyn_cast<SymbolExtent>(*I)) {<br> if (const Decl *D = weakDeclForSymbol(SE))<br> // We have a weakly linked function, return its introduced version.<br> return versionIntroduced(D);<br> } else if (const SymbolConjured *SConj = dyn_cast<SymbolConjured>(*I)) {</blockquote><blockquote type="cite"> const ObjCMessageExpr *ME = dyn_cast<ObjCMessageExpr>(SConj->getStmt());<br> if (!ME)<br> continue;<br><br> // This is an Objective-C message, first get the class availability.<br> // If we have no class here, we cannot reason about the version checking,<br> // so give up.<br> const ObjCInterfaceDecl *CD = ME->getReceiverInterface();<br> if (!CD)<br> break;<br><br> VersionTuple ClassVersion = versionIntroduced(CD);<br><br> // If this message is checking for availability, return the max of the<br> // class version and the checked for method version.<br> Selector MsgSel = ME->getSelector();<br> if (MsgSel.getNumArgs() > 1)<br> break;<br></blockquote><div><br></div><div>Minor, but in general hoist the cheap checks above the expensive ones. Checking if the selector has more than 1 argument is cheaper than getting the version information from the map. This might not matter from a performance perspective, depending on the frequency of which branch succeeds/fails. In this case, ClassVersion isn’t even used until after a bunch of other checks. You can possibly make this more lazy. In this case it probably doesn’t make a difference, so if you prefer to leave it this way that’s fine.</div><br><blockquote type="cite"><br> StringRef SelStr = MsgSel.getNameForSlot(0);<br> if (SelStr == "respondsToSelector" ||<br> SelStr == "instancesRespondToSelector") {<br> // The argument could be a selector expression or a SEL variable.<br> // TODO: make this work for SEL variables.<br> if (const ObjCSelectorExpr *SExp =<br> dyn_cast<ObjCSelectorExpr>(ME->getArg(0))) {<br> Selector ArgSel = SExp->getSelector();<br> const ObjCMethodDecl *D = CD->lookupInstanceMethod(ArgSel);<br> if (!D)<br> D = CD->lookupClassMethod(ArgSel);<br><br> if (D)<br> return std::max(ClassVersion, versionIntroduced(D));<br></blockquote><blockquote type="cite"> }<br> } else if (SelStr == "class") {<br> return ClassVersion;</blockquote><blockquote type="cite"> }<br> }<br> }<br><br> return VersionTuple();<br>}<br><br>VersionTuple<br>UnavailableMethodChecker::getCheckedForVersion(ProgramStateRef State) const {<br> unsigned major = State->get<CheckedVersionMajor>();<br> unsigned minor = State->get<CheckedVersionMinor>();<br> unsigned subminor = State->get<CheckedVersionSubminor>();<br> return VersionTuple(major, minor, sub minor);<br></blockquote><div><br></div><div>Please change this method to do a single lookup for the VersionTuple, instead of three lookups.</div><br><blockquote type="cite">}<br><br>ProgramStateRef<br>UnavailableMethodChecker::setCheckedForVersion(ProgramStateRef State,<br> VersionTuple V) const {<br> VersionTuple CheckedVersion = getCheckedForVersion(State);<br> if (CheckedVersion >= V)<br> return State;<br><br> State = State->set<CheckedVersionMajor>(V.getMajor());<br> State = State->set<CheckedVersionMinor>(V.getMinor());<br> State = State->set<CheckedVersionSubminor>(V.getSubminor());<br> return State;<br></blockquote><div><br></div><div>Please change this method to set the value for one set, instead of 3.</div><br><blockquote type="cite">}<br><br>void ento::registerUnavailableMethodChecker(CheckerManager &mgr) {<br> mgr.registerChecker<UnavailableMethodChecker>();<br></blockquote><div><br></div><div>As mentioned earlier, you can put some checking for deployment information right here. If the context doesn’t support running the checker, just don’t run it at all.</div><br><blockquote type="cite">}<br></blockquote><br></div><div>For the test cases, they look like a good start. One thing I’d like to see is a test case that shows two unsafe calls one after the other. For example, you currently have:</div><div><br></div><div><div><blockquote type="cite"><div>- (void)doVersion30InstanceStuff {</div><div> [super doVersion30InstanceStuff]; // no warning</div><div> [self doVersion40InstanceStuff]; // expected-warning {{Calling method introduced in OS X 40.0 (deployment target is 10.5.0)}}</div><div>}</div></blockquote></div><div><div><br></div></div><div>Try adding a third call here, maybe just repeating the second line:</div><div><br></div><div><blockquote type="cite"><div>- (void)doVersion30InstanceStuff_B {</div><div> [super doVersion30InstanceStuff]; // no warning</div><div> [self doVersion40InstanceStuff]; // expected-warning {{Calling method introduced in OS X 40.0 (deployment target is 10.5.0)}}</div></blockquote><blockquote type="cite"> [self doVersion40InstanceStuff]; // expected-warning {{Calling method introduced in OS X 40.0 (deployment target is 10.5.0)}}</blockquote><blockquote type="cite"><div>}</div></blockquote></div><div><div><br></div></div><div>Right now I’d expect to see two warnings. It would be great to get them down to one (just the first one).</div><br></div><div>Overall this is looking really great. Thanks so much for working on this!</div><div><br></div><div>Cheers,</div><div>Ted</div></body></html>