Bug 186314 - [Cocoa] More preparation for ARC, focusing on WebKit and smart pointers
Summary: [Cocoa] More preparation for ARC, focusing on WebKit and smart pointers
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Darin Adler
URL:
Keywords: InRadar
Depends on:
Blocks: 186385
  Show dependency treegraph
 
Reported: 2018-06-05 12:14 PDT by Darin Adler
Modified: 2018-06-07 09:03 PDT (History)
8 users (show)

See Also:


Attachments
Patch (96.09 KB, patch)
2018-06-05 12:46 PDT, Darin Adler
andersca: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Darin Adler 2018-06-05 12:14:52 PDT
[Cocoa] More preparation for ARC, focusing on WebKit and smart pointers
Comment 1 Darin Adler 2018-06-05 12:46:17 PDT
Created attachment 341984 [details]
Patch
Comment 2 Anders Carlsson 2018-06-05 13:09:51 PDT
Comment on attachment 341984 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=341984&action=review

> Source/WTF/wtf/BlockPtr.h:100
> +#if defined(__OBJC__) && __has_feature(objc_arc)

__has_feature(objc_arc) always implies __OBJC__ being defined, so you don't need to check for __OBJC__ here (or anywhere else).

> Source/WTF/wtf/OSObjectPtr.h:120
> +        OSObjectPtr ptr = other;

I think you can use std::move(other) here - under ARC Objective-C pointers have implicit move constructors.
Comment 3 Darin Adler 2018-06-05 13:57:12 PDT
Comment on attachment 341984 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=341984&action=review

>> Source/WTF/wtf/BlockPtr.h:100
>> +#if defined(__OBJC__) && __has_feature(objc_arc)
> 
> __has_feature(objc_arc) always implies __OBJC__ being defined, so you don't need to check for __OBJC__ here (or anywhere else).

That’s great. I will fix that everywhere. I had copied the #if from RetainPtr. I think I’ll use a separate patch for that.

>> Source/WTF/wtf/OSObjectPtr.h:120
>> +        OSObjectPtr ptr = other;
> 
> I think you can use std::move(other) here - under ARC Objective-C pointers have implicit move constructors.

That’s great news. Given that, maybe I should do a version that takes T&& too? For now I won’t do either of those things, but I will come back and do it as an optimization. Maybe with a unit test.
Comment 4 Darin Adler 2018-06-05 13:58:23 PDT
Committed r232520: <https://trac.webkit.org/changeset/232520>
Comment 5 Radar WebKit Bug Importer 2018-06-05 13:59:19 PDT
<rdar://problem/40822335>
Comment 6 mitz 2018-06-06 23:40:22 PDT
Comment on attachment 341984 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=341984&action=review

> Source/WebKit/Shared/mac/WebCoreArgumentCodersMac.mm:482
> -    IPC::encode(encoder, reinterpret_cast<CFDataRef>(archiver.get().encodedData));
> +    IPC::encode(encoder, (__bridge CFArrayRef)archiver.get().encodedData);

OOPS! This caused bug 186385.
Comment 7 Darin Adler 2018-06-07 09:03:47 PDT
Comment on attachment 341984 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=341984&action=review

>> Source/WebKit/Shared/mac/WebCoreArgumentCodersMac.mm:482
>> +    IPC::encode(encoder, (__bridge CFArrayRef)archiver.get().encodedData);
> 
> OOPS! This caused bug 186385.

Darn, why didn’t I spot that!?