[webkit-reviews] review granted: [Bug 136280] Use RetainPtr::autorelease in some places where it seems appropriate : [Attachment 237213] Version that actually compiles

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Maciej Stachowiak <mjs at apple.com>'s
request for review:
Bug 136280: Use RetainPtr::autorelease in some places where it seems
appropriate
https://bugs.webkit.org/show_bug.cgi?id=136280

Attachment 237213: Version that actually compiles
https://bugs.webkit.org/attachment.cgi?id=237213&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=237213&action=review


> Source/JavaScriptCore/API/JSContext.mm:195
> +    return (NSString *)adoptCF(JSStringCopyCFString(kCFAllocatorDefault,
name)).autorelease();

Seeing all these type casts makes it seem to me that the return type from
autorelease() is not quite right. It’s perfect for functions that return
autoreleased CF objects, which is almost never an idiom. It’s not great for
functions that return autoreleased NS objects due to toll free bridging, which
is what will really happen.

I’d like to see a function like autorelease that only works for types that have
toll free bridging and automatically returns the right type, so we can omit all
these casts and have type checking. Or one that returns (id). We might still
want one that returns the unmolested CF type, but I’m not sure we ever really
need that in practice. Maybe we can turn autorelease into that.

One further note. The current autorelease() implementation, if ever used under
ARC, would only be good if the result was going into an NS type. If we ever
actually tried to use it with the CF type I suspect it would do the wrong thing
and not actually autorelease. Yet another argument for changing the return type
of the function to an NS type.