Summary: | Allow RefPtrs of const types | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alex Christensen <achristensen> | ||||||||
Component: | New Bugs | Assignee: | Alex Christensen <achristensen> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | andersca, benjamin, bfulgham, cdumez, cmarcelo, commit-queue, darin, dbates, mcatanzaro, sam | ||||||||
Priority: | P2 | ||||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Alex Christensen
2016-06-01 13:39:03 PDT
Created attachment 280262 [details]
Patch
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? Created attachment 280315 [details]
Patch
A change was needed, but the const_cast was not. I wonder if we should make RefPtr do the cast instead. 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. (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! I’m OK with this change, but I’d like to see test coverage for this in Tools/TestWebKitAPI/Tests/WTF. Created attachment 280713 [details]
Patch
(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. (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? 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.
(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. Comment on attachment 280713 [details] Patch Clearing flags on attachment: 280713 Committed r203257: <http://trac.webkit.org/changeset/203257> All reviewed patches have been landed. Closing bug. |