Bug 120610 - Need a way to initialize Ref<T> efficiently with ::create() methods.
Summary: Need a way to initialize Ref<T> efficiently with ::create() methods.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andreas Kling
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-09-02 15:05 PDT by Andreas Kling
Modified: 2013-12-05 02:07 PST (History)
4 users (show)

See Also:


Attachments
EWS experiment (11.27 KB, patch)
2013-09-02 15:06 PDT, Andreas Kling
no flags Details | Formatted Diff | Diff
EWS experiment 2 (11.87 KB, patch)
2013-09-02 15:41 PDT, Andreas Kling
no flags Details | Formatted Diff | Diff
Patch sans ChangeLog (13.92 KB, patch)
2013-09-02 17:03 PDT, Andreas Kling
no flags Details | Formatted Diff | Diff
Patch idea. (15.68 KB, patch)
2013-09-02 17:42 PDT, Andreas Kling
no flags Details | Formatted Diff | Diff
Patch (14.79 KB, patch)
2013-09-02 18:47 PDT, Andreas Kling
no flags Details | Formatted Diff | Diff
Patch (15.67 KB, patch)
2013-09-02 21:45 PDT, Andreas Kling
no flags Details | Formatted Diff | Diff
Patch (8.22 KB, patch)
2013-09-06 20:11 PDT, Andreas Kling
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andreas Kling 2013-09-02 15:05:27 PDT
Let's take this to the next level.
Comment 1 Andreas Kling 2013-09-02 15:06:10 PDT
Created attachment 210311 [details]
EWS experiment
Comment 2 Kenneth Rohde Christiansen 2013-09-02 15:09:51 PDT
Comment on attachment 210311 [details]
EWS experiment

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

> Source/WebCore/page/Frame.h:203
> -        const RefPtr<Settings> m_settings;
> +        Ref<Settings> m_settings;

Maybe I am totally missing something here (probably am) but isn't the naming a bit confusing?

RefPtr - reference counted pointer
Ref    - reference counted reference?

I don't have any better suggestions though :-) RefRef would be terrible.
Comment 3 Andreas Kling 2013-09-02 15:22:12 PDT
(In reply to comment #2)
> (From update of attachment 210311 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=210311&action=review
> 
> > Source/WebCore/page/Frame.h:203
> > -        const RefPtr<Settings> m_settings;
> > +        Ref<Settings> m_settings;
> 
> Maybe I am totally missing something here (probably am) but isn't the naming a bit confusing?
> 
> RefPtr - reference counted pointer
> Ref    - reference counted reference?
> 
> I don't have any better suggestions though :-) RefRef would be terrible.

Yeah RefRef is way too ugly to even consider. :)

I went with "Ref" because it's less wordy than RefPtr, providing a kind of minimalist incentive for using it (while attempting to offset the not-so-minimalist get() syntax.)
Comment 4 Andreas Kling 2013-09-02 15:24:51 PDT
(This patch is broken as-is, I'm just here checking compiler support so far.)
Comment 5 Andreas Kling 2013-09-02 15:41:32 PDT
Created attachment 210313 [details]
EWS experiment 2

Alternate version without move constructors.
Comment 6 Andreas Kling 2013-09-02 17:03:24 PDT
Created attachment 210315 [details]
Patch sans ChangeLog
Comment 7 WebKit Commit Bot 2013-09-02 17:04:51 PDT
Attachment 210315 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/wtf/Ref.h', u'Source/WebCore/ChangeLog', u'Source/WebCore/page/Frame.cpp', u'Source/WebCore/page/Frame.h', u'Source/WebCore/page/Page.cpp', u'Source/WebCore/page/Page.h', u'Source/WebCore/page/Settings.cpp', u'Source/WebCore/page/Settings.h']" exit_code: 1
Source/WebCore/ChangeLog:3:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 8 Darin Adler 2013-09-02 17:05:16 PDT
Comment on attachment 210315 [details]
Patch sans ChangeLog

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

> Source/WTF/wtf/Ref.h:82
> +    const T& get() const { return *m_ptr; }

I don’t think this is right. We don’t need to support const Ref<X> since we have Ref<const X> already.
Comment 9 Darin Adler 2013-09-02 17:07:02 PDT
Comment on attachment 210315 [details]
Patch sans ChangeLog

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

> Source/WTF/wtf/Ref.h:36
> +template<typename T> class PassRef {

I say we call this RefInitializer instead (as we discussed).

> Source/WTF/wtf/Ref.h:83
> +    T& get() { return *m_ptr; }

This should be a const member function.
Comment 10 Andreas Kling 2013-09-02 17:42:27 PDT
Created attachment 210316 [details]
Patch idea.
Comment 11 Andreas Kling 2013-09-02 18:47:20 PDT
Created attachment 210317 [details]
Patch
Comment 12 Andreas Kling 2013-09-02 21:45:41 PDT
Created attachment 210320 [details]
Patch
Comment 13 Andreas Kling 2013-09-06 20:11:50 PDT
Created attachment 210830 [details]
Patch
Comment 14 Andreas Kling 2013-09-07 20:48:40 PDT
Päging Darin Adler for comment.
Comment 15 Andreas Kling 2013-09-08 21:02:28 PDT
05:45:28  * darin reads a patch
05:45:30 < darin> not saying which one
05:46:23 < darin> kling: that patch is all wrong
05:46:28 < darin> it uses #ifdef ASSERT_DISABLED
05:46:35 < kling> oh LOL
05:46:36 < darin> it's idea is supercool
05:46:39 < kling> it's supposed to be #if :(
05:46:48 < darin> I think m_wasAdopted needs to be mutable too
05:46:55 < darin> I could say this in review comments (was starting to)
05:47:13 < darin> it also has something needed only before Vector::append(U&&) so obsolete I think
05:47:27 < kling> we don't have Vector::append(U&&) yet
05:47:37 < kling> only uncheckedAppend()
05:47:49 < darin> oh, OK
05:47:53 < kling> anders was blocked on some GCC segfault :(
05:48:04 < darin> also RefInitializer should use a ref, not a pointer
05:48:15 < darin> otherwise it will be copyable
05:48:16 < kling> !
05:48:24 < darin> hmm, maybe even with a ref it is copyable
05:48:31 < darin> I guess you want it copyable
05:48:35 < darin> you made it copyable on purose
05:48:40 < darin> purpose
05:48:44 < darin> it's assignable too
05:48:55 < darin> you should delete operator=
05:49:04 < darin> kling: this is not good form -- should be in review comments
05:49:07 < darin> but those are my comments
05:49:24  * darin pats self on back even though he was naughty
05:49:34 < kling> that's okay, a little public humiliation is good for patch quality
05:49:45 < darin> also we normally use magic enums instead of bools for special second constructors
05:49:54 < darin> cause true is not pretty
05:49:57 < kling> but but but i was just copying PassRefPtr
05:50:00 < kling> but yeah you're right
Comment 16 Andreas Kling 2013-09-08 21:03:16 PDT
Comment on attachment 210830 [details]
Patch

r- from Darin Darinson.
Comment 17 Andreas Kling 2013-12-05 02:07:33 PST
Darin fixed this with PassRef.