Re: Unavailable method checker

[Richard]

On 22 Oct 2013, at 18:43, Jordan Rose <jordan_r...@apple.com> wrote: Looking good! I do still have a few reservations, though. The main one is this: +    if (const SymIntExpr *SIExpr = dyn_cast<SymIntExpr>(*I)) { +      // A symbol is being compared to an int value. The Assumption value +      // is adjusted so that it is checking the symbol exists. +      BinaryOperatorKind op = SIExpr->getOpcode(); +      bool rhs = SIExpr->getRHS() != 0; +      if ((op == BO_EQ && rhs == false) || (op == BO_NE && rhs == true)) +        Assumption = !Assumption; + +      continue; +    } 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: NSAppKitVersionNumber > NSAppKitVersionNumber10_7 At the very least, you should be checking that the opcode is one of the operators you expect. Oops, yes, that was careless. This part of the code handles situations like: if ([obj respondsToSelector:@selector(doVersion20Stuff)] == YES) {     // do version 20 stuff } If the Assumption bool was not matched to the boolean operator, then the following case would also set version check status: if ([obj respondsToSelector:@selector(doVersion20Stuff)] == NO) {     // don't do version 20 stuff } Besides that, here are a storm of little notes: +    if (ExplodedNode *N = C.generateSink(StNull, 0, &Tag)) { +        ImplicitNullDerefEvent Event = { L, false, N, &C.getBugReporter() }; +        dispatchEvent(Event); +    } Extra indentation here. +  if (!(isa<SimpleCall>(&Call) || isa<ObjCMethodCall>(&Call))) isa<> should work on references as well as pointers. +  // If we have an objc method, check for the classes introduced version. +  if (const ObjCMethodDecl *MD = dyn_cast<ObjCMethodDecl>(D)) { +    VersionTuple ClassIntroduced; +    const DeclContext *DC = MD->getDeclContext(); +    if (const ObjCProtocolDecl *PD = dyn_cast<ObjCProtocolDecl>(DC)) +      ClassIntroduced = versionIntroduced(PD); +    else +      ClassIntroduced = versionIntroduced(MD->getClassInterface()); + +    Introduced = std::max(Introduced, ClassIntroduced); +  } 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. Also, typos/formatting: "ObjC" (or full "Objective-C"), and "class's". Nice handling of protocols/categories/implementation/interface. +  // Walk through the symbols children looking for a version check. Typo: "symbol's" +  // If this is a weak function, it is not OK to derefence if availability +  // has not been checked for. Typo: "dereference" +  os << "The method is introduced in "; +  os << Introduced; +  os << ", later than the deployment target "; +  os << deploymentTarget(State); + +  BugReport *R = new BugReport(*BT, os.str(), N); +  BR->emitReport(R); 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 "Calling method introduced in X.Y (deployment target is X.W)"? It's also too bad that there's no highlight range here, but I guess the location is accurate enough. 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. +const Decl* UnavailableMethodChecker::weakDeclForSymbol(SymbolRef Sym) const { Asterisk on the right, please.  +const Decl* +UnavailableMethodChecker::weakDeclForRegion(const MemRegion *R) const { ...and here too. +    } + +    else if (const SymbolConjured *SConj = dyn_cast<SymbolConjured>(*I)) { Please keep these on one line. If it looks too crowded, you can add a blank line before the close brace. +      // This is an objective C message, first get the class availability. Formatting: "Objective-C" +      std::string SelStr = ME->getSelector().getAsString(); +      if (SelStr == "respondsToSelector:" || +          SelStr == "instancesRespondToSelector:") { 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. 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. +        const ObjCSelectorExpr *SelExpr = cast<ObjCSelectorExpr>(ME->getArg(0)); This could fail if the user passes a SEL variable. 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(). +    // We are only checking for SymInt and Symbol comparisons +    else +      break; I'm not sure you want to break...perhaps just ignore these? 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. Thanks for working on this; sorry for drawing it out even more! Jordan So, latest patch attached, I hope I got all the aesthetic things sorted. Richard

unavailable-2410.patch
Description: Binary data

_______________________________________________
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits