Allow RefPtrs of const types

[Alex Christensen]

Allow RefPtrs of const types

[Alex Christensen]

Created attachment 280262 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 280262 [details]
Patch

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

> Source/WTF/wtf/RefCounted.h:145
> +            delete const_cast<T*>(static_cast<const T*>(this));

Was this change actually needed?

[Alex Christensen]

Created attachment 280315 [details]
Patch

[Alex Christensen]

A change was needed, but the const_cast was not.

[Anders Carlsson]

I wonder if we should make RefPtr do the cast instead.

[Alex Christensen]

This allows you to make a RefPtr<const X> but making a RefPtr<X> from a RefPtr<const X> doesn't work because it sees those types as unrelated.

[Darin Adler]

(In reply to comment #6)
> This allows you to make a RefPtr<const X> but making a RefPtr<X> from a
> RefPtr<const X> doesn't work because it sees those types as unrelated.

I think you mean that making a RefPtr<const X> from a RefPtr<X> doesn’t work. Because that other one should not work!

[Darin Adler]

I’m OK with this change, but I’d like to see test coverage for this in Tools/TestWebKitAPI/Tests/WTF.

[Alex Christensen]

Created attachment 280713 [details]
Patch

[Alex Christensen]

(In reply to comment #7)
> (In reply to comment #6)
> > This allows you to make a RefPtr<const X> but making a RefPtr<X> from a
> > RefPtr<const X> doesn't work because it sees those types as unrelated.
> 
> I think you mean that making a RefPtr<const X> from a RefPtr<X> doesn’t
> work. Because that other one should not work!
Yep, I got those backwards.  I thought I was running into template weirdness when using this, but I added a test and it compiles fine.  I'll fix it if I run into it again and something is actually wrong.

[Darin Adler]

(In reply to comment #5)
> I wonder if we should make RefPtr do the cast instead.

Did you have a reply to Anders about this, Alex?

[Brent Fulgham]

Comment on attachment 280713 [details]
Patch

This seems like a good change, but I'd feel better if Anders or Sam glanced at it before we landed it.

[Alex Christensen]

(In reply to comment #11)
> (In reply to comment #5)
> > I wonder if we should make RefPtr do the cast instead.
> 
> Did you have a reply to Anders about this, Alex?

I prefer this approach because RefCounted changing the ref count and deleting the object when the last ref is gone does not mutate the object itself, but if you implemented your own ref/deref functions that did mutate the object significantly, then you would not want a RefPtr<const YourType> to be possible.

[WebKit Commit Bot]

Comment on attachment 280713 [details]
Patch

Clearing flags on attachment: 280713

Committed r203257: <http://trac.webkit.org/changeset/203257>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.