Unavailable method checker
tarka.t.otter at gmail.com
Wed Nov 6 07:18:35 PST 2013
On Tue, Nov 5, 2013 at 6:46 PM, Jordan Rose <jordan_rose at apple.com> wrote:
> On Oct 31, 2013, at 5:43 , Richard <tarka.t.otter at gmail.com> wrote:
> On 29 Oct 2013, at 17:11, Jordan Rose <jordan_rose at apple.com> 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?
> 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:
> 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.
OK, I think the current patch covers everything then. Let me know if you
see any more issues.
-------------- next part --------------
An HTML attachment was scrubbed...
-------------- next part --------------
A non-text attachment was scrubbed...
Size: 21427 bytes
Desc: not available
More information about the cfe-commits