Bug 136280

Summary: Use RetainPtr::autorelease in some places where it seems appropriate
Product: WebKit Reporter: Maciej Stachowiak <mjs>
Component: New BugsAssignee: Maciej Stachowiak <mjs>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, benjamin, cmarcelo, commit-queue, darin
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Version that actually compiles darin: review+

Description Maciej Stachowiak 2014-08-26 21:46:19 PDT
Use RetainPtr::autorelease in some places where it seems appropriate
Comment 1 Maciej Stachowiak 2014-08-26 21:49:59 PDT
Created attachment 237201 [details]
Patch
Comment 2 Maciej Stachowiak 2014-08-27 00:32:32 PDT
Created attachment 237213 [details]
Version that actually compiles
Comment 3 Darin Adler 2014-08-27 11:47:04 PDT
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.
Comment 4 Maciej Stachowiak 2014-08-27 18:04:41 PDT
(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?
Comment 5 Darin Adler 2014-08-28 09:38:59 PDT
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.
Comment 6 Maciej Stachowiak 2014-08-30 15:44:23 PDT
Committed r173141: <http://trac.webkit.org/changeset/173141>