Bug 74440 – RefPtr Basics page should talk about OwnPtr

• Status: RESOLVED
• Resolution: WONTFIX
• Component: WebKit Website

[Joe Mason]

I propose renaming the page to "Smart Pointer Basics" and adding a section on OwnPtr after RefPtr.

[Joe Mason]

Before I do this, let me make sure I have my facts right:

The difference between OwnPtr and PassOwnPtr (apart from paralleling RefPtr and PassRefPtr) is that it's not possible to copy an OwnPtr to an OwnPtr.  Whenever ownership is passed off, PassOwnPtr must be used.  Correct?

Is it correct to say the main reason for this is that the semantics of an OwnPtr becoming 0 after assignment are surprising?  (For instance, if holding the private pimpl structure in an OwnPtr, you would not want anyone to accidentally clear it - this OwnPtr should never be copied.)

[Darin Adler]

I think we should have a separate document about OwnPtr.

[Darin Adler]

(In reply to comment #1)
> The difference between OwnPtr and PassOwnPtr (apart from paralleling RefPtr and PassRefPtr) is that it's not possible to copy an OwnPtr to an OwnPtr. Whenever ownership is passed off, PassOwnPtr must be used. Correct?

Yes.

> Is it correct to say the main reason for this is that the semantics of an OwnPtr becoming 0 after assignment are surprising? (For instance, if holding the private pimpl structure in an OwnPtr, you would not want anyone to accidentally clear it - this OwnPtr should never be copied.)

That’s right. I would not word it quite the same way, but that’s exactly it.

[Mark Rowe (bdash)]

*** Bug 74470 has been marked as a duplicate of this bug. ***

[Naiem]

http://spinwhale.blogspot.com/2011/12/ownptr-and-passownptr.html can some one review this and if it is good we can get it in the webkit.org

[Joe Mason]

Created attachment 120049 [details]
new doc file

[WebKit Review Bot]

Attachment 120049 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Websites/webkit.org/ChangeLog', u'Websites..." exit_code: 1

Websites/webkit.org/ChangeLog:3:  Line contains tab character.  [whitespace/tab] [5]
Websites/webkit.org/ChangeLog:4:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 2 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Joe Mason]

(In reply to comment #5)
> http://spinwhale.blogspot.com/2011/12/ownptr-and-passownptr.html can some one review this and if it is good we can get it in the webkit.org

The patch I just attached explains more than this does, but I did look at this file for ideas.

(BTW, there's an error in that blog post - it says "A leakPtr on PassOwnPtr gives you a PassOwnPtr", which is not true. leakPtr always returns a raw pointer.)

[Joe Mason]

Created attachment 120051 [details]
fixed style error

[Naiem]

(In reply to comment #8)
> (In reply to comment #5)
> > http://spinwhale.blogspot.com/2011/12/ownptr-and-passownptr.html can some one review this and if it is good we can get it in the webkit.org
> 
> The patch I just attached explains more than this does, but I did look at this file for ideas.
> 
> (BTW, there's an error in that blog post - it says "A leakPtr on PassOwnPtr gives you a PassOwnPtr", which is not true. leakPtr always returns a raw pointer.)

my bad fixed it.

[Joe Mason]

Ping?  This has been sitting unreviewed for quite a while.

[Eric Seidel (no email)]

Comment on attachment 120051 [details]
fixed style error

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

> Websites/webkit.org/coding/OwnPtr.html:34
> +by the <span class="class">OwnPtr</span> is automatically deleted when the
> +<span class="class">OwnPtr</span> object goes out of scope. The <span

More generally, when the OwnPtr is itself deconstructed, but this is OK...

> Websites/webkit.org/coding/OwnPtr.html:86
> +    : m_data(new DocumentPrivate)

Moreover, we plan to make this a style error.  There is a bug on file somewhere.

> Websites/webkit.org/coding/OwnPtr.html:130
> +<span class="comment">// legacy function taking a raw pointer</span>
> +void printNode(const Node* node);

This is not necessarily legacy.  It's normal to pass around raw pointers, when there is no expected ownership transfer.  We also don't generally use const with RefCounted classes, like Node, as you can't then refcount them (which may be your intended implication from this?)

> Websites/webkit.org/coding/OwnPtr.html:133
> +void destroyNode(Node* node);

This is confusing because Node* exists in WebCore and is a RefCounted type.  I would chose a different name.

> Websites/webkit.org/coding/OwnPtr.html:152
> +<p>You can force an <span class="class">OwnPtr</span> to give up ownership of a
> +raw pointer by calling the <span class="function">leakPtr</span> function. The
> +caller of this function must arrange for the raw pointer to be deleted in
> +another way. After the raw pointer has been leaked, the <span
> +class="class">OwnPtr</span> is set to 0, since it no longer owns the
> +pointer.</p>

I think I would have structured this paragraph slightly differently. something like this:

You can take ownership from an OwnPtr, using leakPtr(), which will null out the OwnPtr, as well as return the raw pointer it pointed to.  The caller is then responsible for calling delete() on the returned raw pointer.

I might mention here also that .release() is the preferred method of taking ownership from an OwnPtr, as it returns a PassRefPtr, which you're about to cover.

> Websites/webkit.org/coding/OwnPtr.html:172
> +<p><span class="function">adoptPtr</span> and <span
> +class="function">leakPtr</span> can be used together to pass ownership of an
> +object from one <span class="class">OwnPtr</span> to another: the first pointer
> +leaks the object and the second adopts it.</p>

.release() is the way to do this, no?

> Websites/webkit.org/coding/OwnPtr.html:184
> +    m_title = adoptPtr(title.leakPtr());

.release() is the right way.

[Antonio Gomes]

OwnPtr usage is now gone. WONTFIX?