Bug 103851 – createRareData should return a PassOwnPtr

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

[Elliott Sprehn]

Don't use an OwnPtr for Node rare data

[Elliott Sprehn]

Created attachment 177174 [details]
Patch

[Elliott Sprehn]

Created attachment 177175 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 177175 [details]
Patch

This should have been PassOwnPtr anyway, right?  I think it's good to use PassOwnPtr here, even if we always call leakPtr.

[Elliott Sprehn]

(In reply to comment #3)
> (From update of attachment 177175 [details])
> This should have been PassOwnPtr anyway, right?  I think it's good to use PassOwnPtr here, even if we always call leakPtr.

I guess if we want to wrap it then it should be PassOwnPtr though I'm not sure there's an advantage of using PassOwnPtr vs OwnPtr.

What's the reason for wanting to wrap it?

[Eric Seidel (no email)]

(In reply to comment #4)
> (In reply to comment #3)
> > (From update of attachment 177175 [details] [details])
> > This should have been PassOwnPtr anyway, right?  I think it's good to use PassOwnPtr here, even if we always call leakPtr.
> 
> I guess if we want to wrap it then it should be PassOwnPtr though I'm not sure there's an advantage of using PassOwnPtr vs OwnPtr.
> 
> What's the reason for wanting to wrap it?

The goal of wrapping it is the same as any use of smartpointers. :)  To make the ownership clear and prevent memory leaks.  it just so happens that every caller calls leakPtr() today, but this being OwnPtr/PassOwnPtr prevents any future leaks.

[Darin Adler]

Comment on attachment 177175 [details]
Patch

This change doesn’t seem good. Using a return type of PassOwnPtr is an excellent way to make it clear to people reading the code what the lifetime of a returned object is, even if we use leakPtr right after the fact. Moving to a raw pointer does not seem like an improvement, unless there is some concrete benefit of doing so.

Using a return type of OwnPtr is incorrect, though, and should be fixed.

[Elliott Sprehn]

Created attachment 177285 [details]
Patch

[Elliott Sprehn]

(In reply to comment #6)
> (From update of attachment 177175 [details])
> This change doesn’t seem good. Using a return type of PassOwnPtr is an excellent way to make it clear to people reading the code what the lifetime of a returned object is, even if we use leakPtr right after the fact. Moving to a raw pointer does not seem like an improvement, unless there is some concrete benefit of doing so.
> 
> Using a return type of OwnPtr is incorrect, though, and should be fixed.

Okay, patch uploaded with these changes.

[Eric Seidel (no email)]

Comment on attachment 177285 [details]
Patch

I don't see why you're adding the ASSERT.  That method, as written, doesn't need to care about whether it has rare data or not yet.  setRareData might care, but this method seems idempotent as previously written.

[Eric Seidel (no email)]

Comment on attachment 177285 [details]
Patch

Please consider removing the ASSERT.

[Elliott Sprehn]

(In reply to comment #10)
> (From update of attachment 177285 [details])
> Please consider removing the ASSERT.

Okay I'll remove the assert.

[Elliott Sprehn]

Created attachment 177294 [details]
Patch for landing

[Dirk Pranke]

Comment on attachment 177294 [details]
Patch for landing

Clearing flags on attachment: 177294

Committed r136573: <http://trac.webkit.org/changeset/136573>

[Dirk Pranke]

All reviewed patches have been landed.  Closing bug.