Unavailable method checker
Richard
tarka.t.otter at gmail.com
Thu Oct 24 01:19:13 PDT 2013
On 22 Oct 2013, at 18:43, Jordan Rose <jordan_rose at 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131024/29511793/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: unavailable-2410.patch
Type: application/octet-stream
Size: 18692 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131024/29511793/attachment.obj>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131024/29511793/attachment-0001.html>
More information about the cfe-commits
mailing list