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
unavailable-0611.patch
Description: Binary data
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
