hi all, any update on this?
ta, richard. On 08 Nov 2013, at 22:23, Jordan Rose <[email protected]> wrote: > > On Nov 6, 2013, at 7:18 , Tarka T <[email protected]> wrote: > >> On Tue, Nov 5, 2013 at 6:46 PM, Jordan Rose <[email protected]> wrote: >> >> On Oct 31, 2013, at 5:43 , Richard <[email protected]> wrote: >> >>> On 29 Oct 2013, at 17:11, Jordan Rose <[email protected]> wrote: >>> >>>> The logic all seems correct here, but I think we can still do better on >>>> the message. The class and protocol cases seem a little underinformative >>>> to me, especially since the analyzer doesn't have a note where the >>>> availability shows up. So instead of a version kind, how about just >>>> passing the decl that has the availability attribute? That way we can say >>>> "method", "function", "class 'Foo'", or "protocol 'Foo'". >>>> >>>> Also, it would be nice to include the platform name with the version. This >>>> code is stolen from DeclBase.cpp, but I don't have a better place for it >>>> right now. >>>> >>>> StringRef TargetPlatform = Context.getTargetInfo().getPlatformName(); >>>> StringRef PrettyPlatformName >>>> = AvailabilityAttr::getPrettyPlatformName(TargetPlatform); >>>> if (PrettyPlatformName.empty()) >>>> PrettyPlatformName = TargetPlatform; >>>> >>>> "Calling method introduced in OS X 10.9 (deployment target is 10.8)" >>>> >>>> But other than this polish this is looking very good. Have you tried >>>> running it on existing projects? >>>> Jordan >>> >>> OK, I have changed the message as suggested, works for you? >>> >>> I have been running this on some small projects, but today gave it a test >>> on all the large old projects I have here. Mostly it is working well (and >>> caught many unsafe calls I was not aware of), but there were some issues >>> which I have addressed in the latest patch: >>> >>> * id types cause false positives >>> When calling methods on id types (in block enumerations and so on) the >>> method decl match is often incorrect, and so the version check information >>> often causes false positives. I fixed this by not checking any >>> ObjCMethodCall without a receiver interface. >> >> Ha, good catch. >> >> >>> * Array and Dictionary subscripting >>> When using subscripting in a project with a deployment target less than >>> iOS6 (or 10.8?), every subscript will cause a version warning. However, as >>> stated here: >>> >>> https://developer.apple.com/library/ios/releasenotes/ObjectiveC/ObjCAvailabilityIndex/index.html >>> >>> subscripting is handled back to iOS5 and 10.6. I am not sure of the best >>> way to handle this, the current workaround was just to match the selectors >>> by name and not version check when we have a subscript selector. Ideally >>> the checker would make sure that the deployment target was high enough for >>> the runtime to support subscripting, is there a way to get this information >>> from some API? >> >> ObjCMethodCall has an accessor that lets you know if it's using subscripting >> syntax. I think we can ignore it in that case. >> >> >>> * Methods that call [super sameMethod] >>> I had a few false positives where implementing eg >>> >>> - (void)encodeRestorableStateWithCoder:(NSCoder *)coder >>> { >>> [super encodeRestorableStateWithCoder:coder]; >>> } >>> >>> Currently I have left these as false positives, but perhaps there should be >>> special casing to handle this situation too. What do you think about this, >>> is it safe to ignore methods called on super that have been implemented on >>> self, regardless of deployment target? >> >> The most pedantic thing to do here would be to require putting an >> availability attribute on your own method, but I don't think that will fly. >> Let's start conservative and say it's always okay to call super if the >> caller has the same selector as the callee. (In most other cases you'd just >> use self.) >> >> >> Spot comments: >> >> + const Decl *VD = VersionDecl == NULL ? D : VersionDecl; >> >> Might as well just set VersionDecl to D initially. It's not used for >> anything else. >> >> + os << "'" << cast<NamedDecl>(D)->getName() << "'"; >> >> You should just be able to use "os << *D" here, which will work even if we >> start printing method names (which aren't simple identifiers). You can drop >> the cast by making the parameter always be NamedDecl. >> >> + default: >> + break; >> >> Please make this llvm_unreachable, since we don't expect any other kinds to >> come in here and won't handle them properly if they do. >> >> Everything else looks good. Thanks for testing this on existing projects. >> Jordan >> >> OK, I think the current patch covers everything then. Let me know if you see >> any more issues. >> >> Regards, >> Richard > > This looks good to me. I'd like to give Ted a chance to look at it too before > committing, in case he has any comments. Unfortunately, we were tied up with > the Developer Meeting on Wed and Thu, and he's on vacation today, so that > won't be until next week. > > Jordan
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
