Bug 136280 – Use RetainPtr::autorelease in some places where it seems appropriate

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Maciej Stachowiak]

Use RetainPtr::autorelease in some places where it seems appropriate

[Maciej Stachowiak]

Created attachment 237201 [details]
Patch

[Maciej Stachowiak]

Created attachment 237213 [details]
Version that actually compiles

[Darin Adler]

Comment on attachment 237213 [details]
Version that actually compiles

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.

[Maciej Stachowiak]

(In reply to comment #3)
> (From update of attachment 237213 [details])
> 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.

Do we have a generic way to map a CF type to its corresponding NS type?

[Darin Adler]

Comment on attachment 237213 [details]
Version that actually compiles

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.
> 
> Do we have a generic way to map a CF type to its corresponding NS type?

Anders prototyped something called TollFreeBridging.h but he has not yet proposed adding it to WebKit.

[Maciej Stachowiak]

Committed r173141: <http://trac.webkit.org/changeset/173141>