Let is<T>() and downcast<T>() accept RefPtrs/Refs.
<rdar://problem/35102004>
Created attachment 324480 [details] Patch
Attachment 324480 [details] did not pass style-queue: ERROR: Source/WebCore/editing/EditingStyle.cpp:1710: Multi line control clauses should use braces. [whitespace/braces] [4] Total errors found: 1 in 97 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 324480 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=324480&action=review > Source/WTF/ChangeLog:11 > + safecasting methods to accept RefPtrs and Refs. Noted, downcast<T>() returns raw > + pointers for RefPtr and raw references for Ref to avoid ref churn. Is it not possible to have downcast<T>() return a RefPtr<T> or Ref<T> created by adoption?
Comment on attachment 324480 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=324480&action=review >> Source/WTF/ChangeLog:11 >> + pointers for RefPtr and raw references for Ref to avoid ref churn. > > Is it not possible to have downcast<T>() return a RefPtr<T> or Ref<T> created by adoption? Yeah, that seems bad. We shouldn't be returning a raw pointer/reference.
(In reply to Ryosuke Niwa from comment #5) > Comment on attachment 324480 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=324480&action=review > > >> Source/WTF/ChangeLog:11 > >> + pointers for RefPtr and raw references for Ref to avoid ref churn. > > > > Is it not possible to have downcast<T>() return a RefPtr<T> or Ref<T> created by adoption? > > Yeah, that seems bad. We shouldn't be returning a raw pointer/reference. Then are we okay with the unnecessary ref counting? If so, I am willing to change them return RefPtr<T> and Ref<T>.
(In reply to Jiewen Tan from comment #6) > (In reply to Ryosuke Niwa from comment #5) > > Comment on attachment 324480 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=324480&action=review > > > > >> Source/WTF/ChangeLog:11 > > >> + pointers for RefPtr and raw references for Ref to avoid ref churn. > > > > > > Is it not possible to have downcast<T>() return a RefPtr<T> or Ref<T> created by adoption? > > > > Yeah, that seems bad. We shouldn't be returning a raw pointer/reference. > > Then are we okay with the unnecessary ref counting? If so, I am willing to > change them return RefPtr<T> and Ref<T>. You should be able to avoid ref counting churn. Did you look at static_pointer_cast ?
(In reply to Chris Dumez from comment #7) > (In reply to Jiewen Tan from comment #6) > > (In reply to Ryosuke Niwa from comment #5) > > > Comment on attachment 324480 [details] > > > Patch > > > > > > View in context: > > > https://bugs.webkit.org/attachment.cgi?id=324480&action=review > > > > > > >> Source/WTF/ChangeLog:11 > > > >> + pointers for RefPtr and raw references for Ref to avoid ref churn. > > > > > > > > Is it not possible to have downcast<T>() return a RefPtr<T> or Ref<T> created by adoption? > > > > > > Yeah, that seems bad. We shouldn't be returning a raw pointer/reference. > > > > Then are we okay with the unnecessary ref counting? If so, I am willing to > > change them return RefPtr<T> and Ref<T>. > > You should be able to avoid ref counting churn. Did you look at > static_pointer_cast ? It is not obvious to me that how it could avoid ref counting churn. Anyway, proceeding with the suggestions.
(In reply to Jiewen Tan from comment #8) > (In reply to Chris Dumez from comment #7) > > (In reply to Jiewen Tan from comment #6) > > > (In reply to Ryosuke Niwa from comment #5) > > > > Comment on attachment 324480 [details] > > > > Patch > > > > > > > > View in context: > > > > https://bugs.webkit.org/attachment.cgi?id=324480&action=review > > > > > > > > >> Source/WTF/ChangeLog:11 > > > > >> + pointers for RefPtr and raw references for Ref to avoid ref churn. > > > > > > > > > > Is it not possible to have downcast<T>() return a RefPtr<T> or Ref<T> created by adoption? > > > > > > > > Yeah, that seems bad. We shouldn't be returning a raw pointer/reference. > > > > > > Then are we okay with the unnecessary ref counting? If so, I am willing to > > > change them return RefPtr<T> and Ref<T>. > > > > You should be able to avoid ref counting churn. Did you look at > > static_pointer_cast ? > > It is not obvious to me that how it could avoid ref counting churn. Anyway, > proceeding with the suggestions. Yeah, it seems I remembered it wrong. I took a look at static_pointer_cast() and it does not avoid ref counting churn.
Comment on attachment 324480 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=324480&action=review >>>>>>> Source/WTF/ChangeLog:11 >>>>>>> + pointers for RefPtr and raw references for Ref to avoid ref churn. >>>>>> >>>>>> Is it not possible to have downcast<T>() return a RefPtr<T> or Ref<T> created by adoption? >>>>> >>>>> Yeah, that seems bad. We shouldn't be returning a raw pointer/reference. >>>> >>>> Then are we okay with the unnecessary ref counting? If so, I am willing to change them return RefPtr<T> and Ref<T>. >>> >>> You should be able to avoid ref counting churn. Did you look at static_pointer_cast ? >> >> It is not obvious to me that how it could avoid ref counting churn. Anyway, proceeding with the suggestions. > > Yeah, it seems I remembered it wrong. I took a look at static_pointer_cast() and it does not avoid ref counting churn. One way to avoid the refcounting churn would be to have a downcast<> that takes in an Ref<>&& / RefPtr<>&& in parameter. static_pointer_cast<>() does this for RenderPtr.
(In reply to Chris Dumez from comment #10) > Comment on attachment 324480 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=324480&action=review > > >>>>>>> Source/WTF/ChangeLog:11 > >>>>>>> + pointers for RefPtr and raw references for Ref to avoid ref churn. > >>>>>> > >>>>>> Is it not possible to have downcast<T>() return a RefPtr<T> or Ref<T> created by adoption? > >>>>> > >>>>> Yeah, that seems bad. We shouldn't be returning a raw pointer/reference. > >>>> > >>>> Then are we okay with the unnecessary ref counting? If so, I am willing to change them return RefPtr<T> and Ref<T>. > >>> > >>> You should be able to avoid ref counting churn. Did you look at static_pointer_cast ? > >> > >> It is not obvious to me that how it could avoid ref counting churn. Anyway, proceeding with the suggestions. > > > > Yeah, it seems I remembered it wrong. I took a look at static_pointer_cast() and it does not avoid ref counting churn. > > One way to avoid the refcounting churn would be to have a downcast<> that > takes in an Ref<>&& / RefPtr<>&& in parameter. static_pointer_cast<>() does > this for RenderPtr. This doesn't sound elegant as the behaviors are different from the normal downcast<>().
By changing the return type, it reveals that the compiler will actually confuse between the newly added: template<typename ExpectedType, typename ArgType> inline Ref<ExpectedType> downcast(Ref<ArgType>& source) { return downcast<ExpectedType>(source.get()); } and the original one. The compiler will call the new one even if the caller passes an ArgType reference. Not sure how to deal with this issue.
(In reply to Jiewen Tan from comment #12) > By changing the return type, it reveals that the compiler will actually > confuse between the newly added: > template<typename ExpectedType, typename ArgType> > inline Ref<ExpectedType> downcast(Ref<ArgType>& source) > { > return downcast<ExpectedType>(source.get()); > } > and the original one. The compiler will call the new one even if the caller > passes an ArgType reference. Not sure how to deal with this issue. Just spoke to Chris, he said this implicit conversion is done on purpose. He is suggesting to have syntax like downcast<Ref<T>>(). However, this doesn't solve my initial problems. I am trying to attack this problem as it is very annoying to add .get() after I have changed the type of a raw pointer to a RefPtr if the pointer is used in a is<T>() and downcast<T>() later on. With the new downcast<Ref<T>>() syntax, I still have to make changes after I have changed the type. Without any other proper solutions, I might just go with the is<T>() alone.
(In reply to Jiewen Tan from comment #13) > (In reply to Jiewen Tan from comment #12) > > By changing the return type, it reveals that the compiler will actually > > confuse between the newly added: > > template<typename ExpectedType, typename ArgType> > > inline Ref<ExpectedType> downcast(Ref<ArgType>& source) > > { > > return downcast<ExpectedType>(source.get()); > > } > > and the original one. The compiler will call the new one even if the caller > > passes an ArgType reference. Not sure how to deal with this issue. > > Just spoke to Chris, he said this implicit conversion is done on purpose. He > is suggesting to have syntax like downcast<Ref<T>>(). > > However, this doesn't solve my initial problems. I am trying to attack this > problem as it is very annoying to add .get() after I have changed the type > of a raw pointer to a RefPtr if the pointer is used in a is<T>() and > downcast<T>() later on. With the new downcast<Ref<T>>() syntax, I still have > to make changes after I have changed the type. > > Without any other proper solutions, I might just go with the is<T>() alone. Sorry, it turns out to be not the case. It is very specific to containers that define iterators as a Ref<T> object.
Created attachment 324729 [details] Patch
Attachment 324729 [details] did not pass style-queue: ERROR: Source/WebCore/editing/EditingStyle.cpp:1710: Multi line control clauses should use braces. [whitespace/braces] [4] Total errors found: 1 in 112 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 324729 [details] Patch Attachment 324729 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/4975456 Number of test failures exceeded the failure limit.
Created attachment 324743 [details] Archive of layout-test-results from ews107 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
I don't think the mac-wk2 is related to my patch as I cannot reproduce any of those failures in my local machine.
Created attachment 324778 [details] Patch
Attachment 324778 [details] did not pass style-queue: ERROR: Source/WebCore/editing/EditingStyle.cpp:1710: Multi line control clauses should use braces. [whitespace/braces] [4] Total errors found: 1 in 112 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 324778 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=324778&action=review > Source/WTF/wtf/Ref.h:251 > +inline const Ref<ExpectedType> downcast(const Ref<ArgType>& source) We should add another variant which takes Ref<ArgType>&& when we can avoid a ref churn. > Source/WTF/wtf/RefPtr.h:260 > +inline const RefPtr<ExpectedType> downcast(const RefPtr<ArgType>& source) Ditto about adding the variant for RefPtr<ArgType>&& > Source/WebCore/accessibility/AccessibilityListBoxOption.h:66 > - HTMLSelectElement* listBoxOptionParentNode() const; > + RefPtr<HTMLSelectElement> listBoxOptionParentNode() const; Looks like this is unrelated change?
Comment on attachment 324778 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=324778&action=review Thanks Ryosuke for reviewing the patch. Besides adding the move version, I have also added WTFMove() to whenever appropriate. >> Source/WTF/wtf/Ref.h:251 >> +inline const Ref<ExpectedType> downcast(const Ref<ArgType>& source) > > We should add another variant which takes Ref<ArgType>&& when we can avoid a ref churn. Sure. >> Source/WTF/wtf/RefPtr.h:260 >> +inline const RefPtr<ExpectedType> downcast(const RefPtr<ArgType>& source) > > Ditto about adding the variant for RefPtr<ArgType>&& Fixed. >> Source/WebCore/accessibility/AccessibilityListBoxOption.h:66 >> + RefPtr<HTMLSelectElement> listBoxOptionParentNode() const; > > Looks like this is unrelated change? When adding .get() after some of the downcast operations, I picked some that can be benefit by changing the return type.
Created attachment 324881 [details] Patch
Attachment 324881 [details] did not pass style-queue: ERROR: Source/WebCore/editing/EditingStyle.cpp:1710: Multi line control clauses should use braces. [whitespace/braces] [4] Total errors found: 1 in 112 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 324881 [details] Patch Attachment 324881 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/4988457 New failing tests: media/track/track-webvtt-tc026-voice.html media/track/track-cue-mutable-fragment.html http/tests/misc/object-embedding-svg-delayed-size-negotiation-2.htm media/track/getCueAsHTMLCrash.html media/track/track-webvtt-tc023-markup.html media/track/track-webvtt-tc025-class-markup.html
Created attachment 324897 [details] Archive of layout-test-results from ews104 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 324881 [details] Patch Attachment 324881 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/4988322 New failing tests: media/track/track-webvtt-tc026-voice.html media/track/getCueAsHTMLCrash.html media/track/track-webvtt-tc023-markup.html media/track/track-cue-mutable-fragment.html media/track/track-webvtt-tc025-class-markup.html
Created attachment 324900 [details] Archive of layout-test-results from ews102 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Created attachment 324908 [details] Patch
Attachment 324908 [details] did not pass style-queue: ERROR: Source/WebCore/editing/EditingStyle.cpp:1710: Multi line control clauses should use braces. [whitespace/braces] [4] Total errors found: 1 in 112 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 324908 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=324908&action=review > Source/WTF/wtf/Ref.h:245 > +inline Ref<ExpectedType> downcast(Ref<ArgType>& source) I've started to think that we might want to call this variant downcastAndCopyRef or something. The fact downcast<~> sometimes implicitly calls ref()'s is very subtle. Another alternative Geoff and I discussed would be for this function to return ExpectedType& or Ref<ExpectedType>&. For the case of Ref<ExpectedType>&, we would be casting Ref<~> itself, not the content of it. > Source/WTF/wtf/Ref.h:251 > +inline const Ref<ExpectedType> downcast(const Ref<ArgType>& source) Ditto. > Source/WTF/wtf/RefPtr.h:254 > +inline RefPtr<ExpectedType> downcast(RefPtr<ArgType>& source) Again, we might want to call this variant downcastAndCopyRef or copyRefToDowncast > Source/WebCore/css/CSSFontFace.cpp:66 > - CSSFontFaceSrcValue& item = downcast<CSSFontFaceSrcValue>(src.get()); > + auto item = downcast<CSSFontFaceSrcValue>(src); This code, for example, calls the variant which ref()'s and it's not obvious at all from the call site. > Source/WebCore/css/CSSFontFaceSet.cpp:158 > - String familyName = CSSFontFaceSet::familyNameFromPrimitive(downcast<CSSPrimitiveValue>(item.get())); > + String familyName = CSSFontFaceSet::familyNameFromPrimitive(downcast<CSSPrimitiveValue>(item)); Again, this copies ref(). > Source/WebCore/css/StyleBuilderConverter.h:392 > + for (const auto& currentValue : downcast<CSSValueList>(value)) It's unfortunate that the type of currentValue, const Ref<CSSValue>&, is not obvious at all from the code inspection. > Source/WebCore/css/StyleBuilderCustom.h:393 > - CSSPrimitiveValue& primitiveValue = downcast<CSSPrimitiveValue>(item.get()); > + CSSPrimitiveValue& primitiveValue = downcast<CSSPrimitiveValue>(item); This seems wrong. Why is there an implicit coercion from Ref<CSSValue> to CSSPrimitiveValue& happening? > Source/WebCore/css/StyleBuilderCustom.h:514 > + auto primitiveValue = downcast<CSSPrimitiveValue>(item); Ditto. > Source/WebCore/css/StyleBuilderCustom.h:836 > - auto& shadowValue = downcast<CSSShadowValue>(item.get()); > + auto shadowValue = downcast<CSSShadowValue>(item); Ditto. > Source/WebCore/css/StyleBuilderCustom.h:940 > - auto& contentValue = downcast<CSSPrimitiveValue>(item.get()); > + auto contentValue = downcast<CSSPrimitiveValue>(item); Ditto.
Comment on attachment 324908 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=324908&action=review >> Source/WTF/wtf/Ref.h:245 >> +inline Ref<ExpectedType> downcast(Ref<ArgType>& source) > > I've started to think that we might want to call this variant downcastAndCopyRef or something. > The fact downcast<~> sometimes implicitly calls ref()'s is very subtle. > > Another alternative Geoff and I discussed would be for this function to return ExpectedType& or Ref<ExpectedType>&. > For the case of Ref<ExpectedType>&, we would be casting Ref<~> itself, not the content of it. I like the idea of returning Ref<ExpectedType>&. Let's proceed with that.
Created attachment 324976 [details] Patch
Attachment 324976 [details] did not pass style-queue: ERROR: Source/WebCore/editing/EditingStyle.cpp:1710: Multi line control clauses should use braces. [whitespace/braces] [4] Total errors found: 1 in 112 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 324976 [details] Patch Attachment 324976 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/4996238 New failing tests: dom/html/level2/core/hc_nodedocumentfragmentnormalize2.html fast/dom/Node/normalize.html dom/xhtml/level2/core/hc_nodedocumentfragmentnormalize2.xhtml imported/w3c/web-platform-tests/dom/nodes/Node-normalize.html
Created attachment 324981 [details] Archive of layout-test-results from ews101 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 324976 [details] Patch Attachment 324976 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/4996313 New failing tests: dom/xhtml/level2/core/hc_nodedocumentfragmentnormalize2.xhtml fast/dom/Node/normalize.html dom/html/level2/core/hc_nodedocumentfragmentnormalize2.html imported/w3c/web-platform-tests/dom/nodes/Node-normalize.html
Created attachment 324983 [details] Archive of layout-test-results from ews114 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews114 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 324976 [details] Patch Attachment 324976 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/4997283 New failing tests: dom/html/level2/core/hc_nodedocumentfragmentnormalize2.html fast/dom/Node/normalize.html dom/xhtml/level2/core/hc_nodedocumentfragmentnormalize2.xhtml imported/w3c/web-platform-tests/dom/nodes/Node-normalize.html
Created attachment 324996 [details] Archive of layout-test-results from ews125 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews125 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.6
Comment on attachment 324976 [details] Patch Attachment 324976 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/4998630 New failing tests: dom/xhtml/level2/core/hc_nodedocumentfragmentnormalize2.xhtml fast/dom/Node/normalize.html dom/html/level2/core/hc_nodedocumentfragmentnormalize2.html imported/w3c/web-platform-tests/dom/nodes/Node-normalize.html
Created attachment 325013 [details] Archive of layout-test-results from ews106 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Created attachment 325042 [details] Patch
Attachment 325042 [details] did not pass style-queue: ERROR: Source/WebCore/editing/EditingStyle.cpp:1710: Multi line control clauses should use braces. [whitespace/braces] [4] Total errors found: 1 in 112 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 325042 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=325042&action=review > Source/WTF/wtf/RefPtr.h:242 > +inline bool is(RefPtr<ArgType>& source) Do we really need this one? isn't the const one below sufficient? > Source/WebCore/accessibility/AccessibilityListBoxOption.cpp:86 > + auto listBoxParentNode = listBoxOptionParentNode(); Not sure we should be making these unrelated changes in this patch. > Source/WebCore/accessibility/AccessibilityListBoxOption.cpp:191 > +RefPtr<HTMLSelectElement> AccessibilityListBoxOption::listBoxOptionParentNode() const Not sure we should be making these unrelated changes in this patch. > Source/WebCore/accessibility/AccessibilityListBoxOption.h:66 > + RefPtr<HTMLSelectElement> listBoxOptionParentNode() const; Not sure we should be making these unrelated changes in this patch. > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2360 > + RefPtr<Node> node = hitTestResult.innerNode(); Not sure we should be making these unrelated changes in this patch. > Source/WebCore/crypto/algorithms/CryptoAlgorithmHMAC.cpp:151 > + const auto hmacKey = downcast<CryptoKeyHMAC>(WTFMove(key)); no need for const. > Source/WebCore/crypto/algorithms/CryptoAlgorithmRSA_OAEP.cpp:199 > + const auto rsaKey = downcast<CryptoKeyRSA>(WTFMove(key)); no need for the const. > Source/WebCore/css/StyleBuilderConverter.h:392 > + for (const auto& currentValue : downcast<CSSValueList>(value)) I don't think we should add those const here. We usually don't have const variables in WebKit. I remember Darin asking me to not use const in such cases too. > Source/WebCore/css/StyleBuilderConverter.h:496 > + for (const auto& currentValue : downcast<CSSValueList>(value)) ditto. > Source/WebCore/css/StyleBuilderConverter.h:534 > + for (const auto& currentValue : downcast<CSSValueList>(value)) { ditto. > Source/WebCore/css/StyleBuilderConverter.h:535 > + const auto& primitiveValue = downcast<CSSPrimitiveValue>(currentValue); ditto. > Source/WebCore/dom/Node.cpp:588 > + auto text = downcast<Text>(node).copyRef(); Is the copyRef() really needed? Doesn't this return a RefPtr?
Comment on attachment 325042 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=325042&action=review > Source/WTF/wtf/RefPtr.h:258 > + RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(is<ExpectedType>(source.get())); So we pay a release-time cost just for casting? > Source/WTF/wtf/RefPtr.h:267 > + RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(is<ExpectedType>(source.get())); So we pay a release-time cost just for casting?
Comment on attachment 325042 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=325042&action=review > Source/WTF/wtf/RefPtr.h:259 > + return reinterpret_cast<RefPtr<ExpectedType>&>(source); Why is this reinterpret_cast safe? What about multiple inheritance? class A { } class B : public RefCounted<B> { } class C : public A, public B { } RefPtr<B> b = adoptRef(new C); RefPtr<C> c = downcast<C>(b); Does't this break because of the reinterpret_cast? I believe the addresses of b and c should be different but would end up being the same in your case due to the reinterpret_cast, no?
Comment on attachment 325042 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=325042&action=review Thanks Chris for reviewing my patch. >> Source/WTF/wtf/RefPtr.h:242 >> +inline bool is(RefPtr<ArgType>& source) > > Do we really need this one? isn't the const one below sufficient? No, it is required. Otherwise, the compiler will then choose the original is<T>() instead of the latter one. >> Source/WTF/wtf/RefPtr.h:258 >> + RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(is<ExpectedType>(source.get())); > > So we pay a release-time cost just for casting? I got warning from our style checker ask me to switch a debug assert to a release assert. Probably, we don't want to enable it for casting at this moment. >> Source/WTF/wtf/RefPtr.h:259 >> + return reinterpret_cast<RefPtr<ExpectedType>&>(source); > > Why is this reinterpret_cast safe? What about multiple inheritance? > > class A { } > class B : public RefCounted<B> { } > > class C : public A, public B { } > > RefPtr<B> b = adoptRef(new C); > RefPtr<C> c = downcast<C>(b); > > Does't this break because of the reinterpret_cast? I believe the addresses of b and c should be different but would end up being the same in your case due to the reinterpret_cast, no? RefPtr<C> c = downcast<C>(b); I don't think the above semantic is allowed after this change. It will actually be: RefPtr<C>& c = downcast<C>(b); Since c is just an alias of b, yes, they share the same address. >> Source/WTF/wtf/RefPtr.h:267 >> + RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(is<ExpectedType>(source.get())); > > So we pay a release-time cost just for casting? Ditto. >> Source/WebCore/accessibility/AccessibilityListBoxOption.cpp:86 >> + auto listBoxParentNode = listBoxOptionParentNode(); > > Not sure we should be making these unrelated changes in this patch. It is a long story. Let's put it in short, some methods return downcast<T>() directly. After this change, downcast<T>() will return a Ref/RefPtr via the move downcast variant. Therefore, I felt it would be awkward to add .get() after downcast<T>(). Therefore, instead of doing that, I change the return type of those methods to Ref/RefPtr. And then, recursion happens. Consequently, I changed methods that return the mentioned before methods directly to Ref/RefPtr. That's how this seem unrelated ends up with. In this cast, it is: ownerSelectElement() that introduces. >> Source/WebCore/accessibility/AccessibilityListBoxOption.cpp:191 >> +RefPtr<HTMLSelectElement> AccessibilityListBoxOption::listBoxOptionParentNode() const > > Not sure we should be making these unrelated changes in this patch. Ditto. >> Source/WebCore/accessibility/AccessibilityListBoxOption.h:66 >> + RefPtr<HTMLSelectElement> listBoxOptionParentNode() const; > > Not sure we should be making these unrelated changes in this patch. Ditto. >> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2360 >> + RefPtr<Node> node = hitTestResult.innerNode(); > > Not sure we should be making these unrelated changes in this patch. I cannot recalled why I did these changes. Discarded... >> Source/WebCore/crypto/algorithms/CryptoAlgorithmHMAC.cpp:151 >> + const auto hmacKey = downcast<CryptoKeyHMAC>(WTFMove(key)); > > no need for const. Removed. >> Source/WebCore/crypto/algorithms/CryptoAlgorithmRSA_OAEP.cpp:199 >> + const auto rsaKey = downcast<CryptoKeyRSA>(WTFMove(key)); > > no need for the const. Ditto. >> Source/WebCore/css/StyleBuilderConverter.h:392 >> + for (const auto& currentValue : downcast<CSSValueList>(value)) > > I don't think we should add those const here. We usually don't have const variables in WebKit. I remember Darin asking me to not use const in such cases too. Removed. >> Source/WebCore/css/StyleBuilderConverter.h:496 >> + for (const auto& currentValue : downcast<CSSValueList>(value)) > > ditto. Ditto. >> Source/WebCore/css/StyleBuilderConverter.h:534 >> + for (const auto& currentValue : downcast<CSSValueList>(value)) { > > ditto. Ditto. >> Source/WebCore/css/StyleBuilderConverter.h:535 >> + const auto& primitiveValue = downcast<CSSPrimitiveValue>(currentValue); > > ditto. Ditto.
Comment on attachment 325042 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=325042&action=review >>> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2360 >>> + RefPtr<Node> node = hitTestResult.innerNode(); >> >> Not sure we should be making these unrelated changes in this patch. > > I cannot recalled why I did these changes. Discarded... Okay, I recalled. We assigned: node = downcast<HTMLOptionElement>(*node).ownerSelectElement(); Therefore, I change node from Node* to RefPtr<Node>.
Comment on attachment 325042 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=325042&action=review >>> Source/WTF/wtf/RefPtr.h:259 >>> + return reinterpret_cast<RefPtr<ExpectedType>&>(source); >> >> Why is this reinterpret_cast safe? What about multiple inheritance? >> >> class A { } >> class B : public RefCounted<B> { } >> >> class C : public A, public B { } >> >> RefPtr<B> b = adoptRef(new C); >> RefPtr<C> c = downcast<C>(b); >> >> Does't this break because of the reinterpret_cast? I believe the addresses of b and c should be different but would end up being the same in your case due to the reinterpret_cast, no? > > RefPtr<C> c = downcast<C>(b); > > I don't think the above semantic is allowed after this change. It will actually be: > RefPtr<C>& c = downcast<C>(b); > > Since c is just an alias of b, yes, they share the same address. I think you understood me wrong: b.get() and c.get() are supposed to return different addresses because of multiple inheritance. However, I believe you may end up with the same address because of your reinterpret_cast. If so, it would likely lead to crashes when trying to call methods inherited from A on c.
Comment on attachment 325042 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=325042&action=review >>>> Source/WTF/wtf/RefPtr.h:259 >>>> + return reinterpret_cast<RefPtr<ExpectedType>&>(source); >>> >>> Why is this reinterpret_cast safe? What about multiple inheritance? >>> >>> class A { } >>> class B : public RefCounted<B> { } >>> >>> class C : public A, public B { } >>> >>> RefPtr<B> b = adoptRef(new C); >>> RefPtr<C> c = downcast<C>(b); >>> >>> Does't this break because of the reinterpret_cast? I believe the addresses of b and c should be different but would end up being the same in your case due to the reinterpret_cast, no? >> >> RefPtr<C> c = downcast<C>(b); >> >> I don't think the above semantic is allowed after this change. It will actually be: >> RefPtr<C>& c = downcast<C>(b); >> >> Since c is just an alias of b, yes, they share the same address. > > I think you understood me wrong: > b.get() and c.get() are supposed to return different addresses because of multiple inheritance. However, I believe you may end up with the same address because of your reinterpret_cast. > If so, it would likely lead to crashes when trying to call methods inherited from A on c. Okay, I got you.
> >> Does't this break because of the reinterpret_cast? I believe the addresses of b and c should be different but would end up being the same in your case due to the reinterpret_cast, no? > > > > RefPtr<C> c = downcast<C>(b); > > > > I don't think the above semantic is allowed after this change. It will actually be: > > RefPtr<C>& c = downcast<C>(b); > > > > Since c is just an alias of b, yes, they share the same address. > > I think you understood me wrong: > b.get() and c.get() are supposed to return different addresses because of > multiple inheritance. However, I believe you may end up with the same > address because of your reinterpret_cast. > If so, it would likely lead to crashes when trying to call methods inherited > from A on c. I wonder if this is also a problem with our implementation of weak_reference_downcast<T, U>.
(In reply to Geoffrey Garen from comment #53) > > >> Does't this break because of the reinterpret_cast? I believe the addresses of b and c should be different but would end up being the same in your case due to the reinterpret_cast, no? > > > > > > RefPtr<C> c = downcast<C>(b); > > > > > > I don't think the above semantic is allowed after this change. It will actually be: > > > RefPtr<C>& c = downcast<C>(b); > > > > > > Since c is just an alias of b, yes, they share the same address. > > > > I think you understood me wrong: > > b.get() and c.get() are supposed to return different addresses because of > > multiple inheritance. However, I believe you may end up with the same > > address because of your reinterpret_cast. > > If so, it would likely lead to crashes when trying to call methods inherited > > from A on c. > > I wonder if this is also a problem with our implementation of > weak_reference_downcast<T, U>. I pointed out the same thing to Jiewen offline :)
(In reply to Geoffrey Garen from comment #53) > I wonder if this is also a problem with our implementation of > weak_reference_downcast<T, U>. This can easily be tested in TestWebKitAPI, so we should add that kind of test.
(In reply to Darin Adler from comment #55) > (In reply to Geoffrey Garen from comment #53) > > I wonder if this is also a problem with our implementation of > > weak_reference_downcast<T, U>. > > This can easily be tested in TestWebKitAPI, so we should add that kind of > test. Test added. Surprisingly, it passes without any crashes. Not sure if I wrote something wrong. Here is the test. enum class Class { Multi, }; struct Base : RefCounted<Base> { virtual Class type() const = 0; virtual ~Base() { } }; struct ExtraBase { int returnOne() { return 1; } }; struct Multi : ExtraBase, Base { virtual Class type() const override { return Class::Multi; } }; TEST(WTF_RefPtr, MultiInherence) { RefPtr<Base> p1 = adoptRef(new Multi); auto& p2 = downcast<Multi>(p1); EXPECT_NE(p1.get(), p2.get()); EXPECT_EQ(p2->returnOne(), 1); } #define SPECIALIZE_TYPE_TRAITS_Base(ToClassName, Class) \ SPECIALIZE_TYPE_TRAITS_BEGIN(TestWebKitAPI::ToClassName) \ static bool isType(const TestWebKitAPI::Base& base) { return base.type() == TestWebKitAPI::Class; } \ SPECIALIZE_TYPE_TRAITS_END() SPECIALIZE_TYPE_TRAITS_Base(Multi, Class::Multi)
(In reply to Jiewen Tan from comment #56) > (In reply to Darin Adler from comment #55) > > (In reply to Geoffrey Garen from comment #53) > > > I wonder if this is also a problem with our implementation of > > > weak_reference_downcast<T, U>. > > > > This can easily be tested in TestWebKitAPI, so we should add that kind of > > test. > > Test added. Surprisingly, it passes without any crashes. Not sure if I wrote > something wrong. Here is the test. > > enum class Class { > Multi, > }; > > struct Base : RefCounted<Base> { > virtual Class type() const = 0; > virtual ~Base() { } > }; > > struct ExtraBase { > int returnOne() { return 1; } > }; > > struct Multi : ExtraBase, Base { > virtual Class type() const override { return Class::Multi; } > }; > > TEST(WTF_RefPtr, MultiInherence) > { > RefPtr<Base> p1 = adoptRef(new Multi); > auto& p2 = downcast<Multi>(p1); > EXPECT_NE(p1.get(), p2.get()); > EXPECT_EQ(p2->returnOne(), 1); > } > > #define SPECIALIZE_TYPE_TRAITS_Base(ToClassName, Class) \ > SPECIALIZE_TYPE_TRAITS_BEGIN(TestWebKitAPI::ToClassName) \ > static bool isType(const TestWebKitAPI::Base& base) { return base.type() == > TestWebKitAPI::Class; } \ > SPECIALIZE_TYPE_TRAITS_END() > > SPECIALIZE_TYPE_TRAITS_Base(Multi, Class::Multi) Sorry, it should be: EXPECT_EQ(p1.get(), p2.get());
(In reply to Jiewen Tan from comment #57) > (In reply to Jiewen Tan from comment #56) > > (In reply to Darin Adler from comment #55) > > > (In reply to Geoffrey Garen from comment #53) > > > > I wonder if this is also a problem with our implementation of > > > > weak_reference_downcast<T, U>. > > > > > > This can easily be tested in TestWebKitAPI, so we should add that kind of > > > test. > > > > Test added. Surprisingly, it passes without any crashes. Not sure if I wrote > > something wrong. Here is the test. > > > > enum class Class { > > Multi, > > }; > > > > struct Base : RefCounted<Base> { > > virtual Class type() const = 0; > > virtual ~Base() { } > > }; > > > > struct ExtraBase { > > int returnOne() { return 1; } > > }; > > > > struct Multi : ExtraBase, Base { > > virtual Class type() const override { return Class::Multi; } > > }; > > > > TEST(WTF_RefPtr, MultiInherence) > > { > > RefPtr<Base> p1 = adoptRef(new Multi); > > auto& p2 = downcast<Multi>(p1); > > EXPECT_NE(p1.get(), p2.get()); > > EXPECT_EQ(p2->returnOne(), 1); > > } > > > > #define SPECIALIZE_TYPE_TRAITS_Base(ToClassName, Class) \ > > SPECIALIZE_TYPE_TRAITS_BEGIN(TestWebKitAPI::ToClassName) \ > > static bool isType(const TestWebKitAPI::Base& base) { return base.type() == > > TestWebKitAPI::Class; } \ > > SPECIALIZE_TYPE_TRAITS_END() > > > > SPECIALIZE_TYPE_TRAITS_Base(Multi, Class::Multi) > > Sorry, it should be: > EXPECT_EQ(p1.get(), p2.get()); I think that if you'd static_cast() then p1.get() and p2.get() would be different.
(In reply to Chris Dumez from comment #58) > (In reply to Jiewen Tan from comment #57) > > (In reply to Jiewen Tan from comment #56) > > > (In reply to Darin Adler from comment #55) > > > > (In reply to Geoffrey Garen from comment #53) > > > > > I wonder if this is also a problem with our implementation of > > > > > weak_reference_downcast<T, U>. > > > > > > > > This can easily be tested in TestWebKitAPI, so we should add that kind of > > > > test. > > > > > > Test added. Surprisingly, it passes without any crashes. Not sure if I wrote > > > something wrong. Here is the test. > > > > > > enum class Class { > > > Multi, > > > }; > > > > > > struct Base : RefCounted<Base> { > > > virtual Class type() const = 0; > > > virtual ~Base() { } > > > }; > > > > > > struct ExtraBase { > > > int returnOne() { return 1; } > > > }; > > > > > > struct Multi : ExtraBase, Base { > > > virtual Class type() const override { return Class::Multi; } > > > }; > > > > > > TEST(WTF_RefPtr, MultiInherence) > > > { > > > RefPtr<Base> p1 = adoptRef(new Multi); > > > auto& p2 = downcast<Multi>(p1); > > > EXPECT_NE(p1.get(), p2.get()); > > > EXPECT_EQ(p2->returnOne(), 1); > > > } > > > > > > #define SPECIALIZE_TYPE_TRAITS_Base(ToClassName, Class) \ > > > SPECIALIZE_TYPE_TRAITS_BEGIN(TestWebKitAPI::ToClassName) \ > > > static bool isType(const TestWebKitAPI::Base& base) { return base.type() == > > > TestWebKitAPI::Class; } \ > > > SPECIALIZE_TYPE_TRAITS_END() > > > > > > SPECIALIZE_TYPE_TRAITS_Base(Multi, Class::Multi) > > > > Sorry, it should be: > > EXPECT_EQ(p1.get(), p2.get()); > > I think that if you'd static_cast() then p1.get() and p2.get() would be > different. No, static_cast() doesn't do the trick. Here is the revised test: TEST(WTF_RefPtr, MultiInherence) { RefPtr<Base> p1 = adoptRef(new Multi); auto& p2 = downcast<Multi>(p1); EXPECT_EQ(p1.get(), p2.get()); EXPECT_EQ(p2->returnOne(), 1); auto* p3 = downcast<Multi>(p1.get()); EXPECT_NE(p1.get(), p3); EXPECT_EQ(p3->returnOne(), 1); } Output: jwtan$ run-api-tests WTF_RefPtr.MultiInherence Running build-api-tests aFAIL WTF_RefPtr.MultiInherence /Users/jwtan/Documents/Source/OpenSource/Tools/TestWebKitAPI/Tests/WTF/RefPtr.cpp:563 Expected: (p1.get()) != (p3), actual: 0x1056fb000 vs 0x1056fb000 Tests that failed: WTF_RefPtr.MultiInherence
(In reply to Jiewen Tan from comment #59) > (In reply to Chris Dumez from comment #58) > > (In reply to Jiewen Tan from comment #57) > > > (In reply to Jiewen Tan from comment #56) > > > > (In reply to Darin Adler from comment #55) > > > > > (In reply to Geoffrey Garen from comment #53) > > > > > > I wonder if this is also a problem with our implementation of > > > > > > weak_reference_downcast<T, U>. > > > > > > > > > > This can easily be tested in TestWebKitAPI, so we should add that kind of > > > > > test. > > > > > > > > Test added. Surprisingly, it passes without any crashes. Not sure if I wrote > > > > something wrong. Here is the test. > > > > > > > > enum class Class { > > > > Multi, > > > > }; > > > > > > > > struct Base : RefCounted<Base> { > > > > virtual Class type() const = 0; > > > > virtual ~Base() { } > > > > }; > > > > > > > > struct ExtraBase { > > > > int returnOne() { return 1; } > > > > }; > > > > > > > > struct Multi : ExtraBase, Base { > > > > virtual Class type() const override { return Class::Multi; } > > > > }; > > > > > > > > TEST(WTF_RefPtr, MultiInherence) > > > > { > > > > RefPtr<Base> p1 = adoptRef(new Multi); > > > > auto& p2 = downcast<Multi>(p1); > > > > EXPECT_NE(p1.get(), p2.get()); > > > > EXPECT_EQ(p2->returnOne(), 1); > > > > } > > > > > > > > #define SPECIALIZE_TYPE_TRAITS_Base(ToClassName, Class) \ > > > > SPECIALIZE_TYPE_TRAITS_BEGIN(TestWebKitAPI::ToClassName) \ > > > > static bool isType(const TestWebKitAPI::Base& base) { return base.type() == > > > > TestWebKitAPI::Class; } \ > > > > SPECIALIZE_TYPE_TRAITS_END() > > > > > > > > SPECIALIZE_TYPE_TRAITS_Base(Multi, Class::Multi) > > > > > > Sorry, it should be: > > > EXPECT_EQ(p1.get(), p2.get()); > > > > I think that if you'd static_cast() then p1.get() and p2.get() would be > > different. > > No, static_cast() doesn't do the trick. Here is the revised test: > TEST(WTF_RefPtr, MultiInherence) > { > RefPtr<Base> p1 = adoptRef(new Multi); > auto& p2 = downcast<Multi>(p1); > EXPECT_EQ(p1.get(), p2.get()); > EXPECT_EQ(p2->returnOne(), 1); > > auto* p3 = downcast<Multi>(p1.get()); > EXPECT_NE(p1.get(), p3); > EXPECT_EQ(p3->returnOne(), 1); > } > > Output: > jwtan$ run-api-tests WTF_RefPtr.MultiInherence > Running build-api-tests > aFAIL WTF_RefPtr.MultiInherence > > /Users/jwtan/Documents/Source/OpenSource/Tools/TestWebKitAPI/Tests/WTF/ > RefPtr.cpp:563 > Expected: (p1.get()) != (p3), actual: 0x1056fb000 vs 0x1056fb000 > > > Tests that failed: > WTF_RefPtr.MultiInherence Okay. Here is a workable test case to demonstrate the problem: enum class Class { Multi, }; struct Base : RefCounted<Base> { virtual Class type() const = 0; virtual ~Base() { } int someData { 0 }; }; struct ExtraBase { virtual ~ExtraBase() { } int someExtraData { 1 }; }; struct Multi : ExtraBase, Base { virtual Class type() const override { return Class::Multi; } }; struct MultiReverse : Base, ExtraBase { virtual Class type() const override { return Class::Multi; } }; TEST(WTF_RefPtr, MultiInherence) { RefPtr<Base> p1 = adoptRef(new Multi()); auto& p2 = downcast<Multi>(p1); EXPECT_EQ((void*)p1.get(), (void*)p2.get()); auto* p3 = downcast<Multi>(p1.get()); EXPECT_NE((void*)p1.get(), (void*)p3); EXPECT_NE((void*)p2.get(), (void*)p3); } TEST(WTF_RefPtr, MultiInherenceReverse) { RefPtr<Base> p1 = adoptRef(new Multi()); auto& p2 = downcast<Multi>(p1); EXPECT_EQ((void*)p1.get(), (void*)p2.get()); } #define SPECIALIZE_TYPE_TRAITS_Base(ToClassName, Class) \ SPECIALIZE_TYPE_TRAITS_BEGIN(TestWebKitAPI::ToClassName) \ static bool isType(const TestWebKitAPI::Base& base) { return base.type() == TestWebKitAPI::Class; } \ SPECIALIZE_TYPE_TRAITS_END() SPECIALIZE_TYPE_TRAITS_Base(Multi, Class::Multi) The current implementation passes all the expectations which demonstrates Chris' comment. With that being said, my take will be making the first two variants return regular pointers and references.
I don’t understand why the test case involves all those casts to (void*).
(In reply to Darin Adler from comment #61) > I don’t understand why the test case involves all those casts to (void*). As far as I understand, when comparing pointers, implicit static casting will apply to one of the operands to make both operands the same type of pointers. To prevent this implicit static casting, my take is to cast them to void pointers.
Created attachment 325544 [details] Patch
Created attachment 325605 [details] Patch
Attachment 325605 [details] did not pass style-queue: ERROR: Source/WebCore/editing/EditingStyle.cpp:1710: Multi line control clauses should use braces. [whitespace/braces] [4] Total errors found: 1 in 109 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 325605 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=325605&action=review The ability to call is<T> without a .get() looks great. This patch includes lots of uses of WTFMove that I don't think belong. I've noted some but not all of them. I think you should fix that. We should only WTFMove when we intend to transfer ownership, and not when we just want to dereference an object and access one of its data members. I'm starting to think that it's bad for downcast<T> to implicitly remove the RefPtr lifetime guarantee. In combination with auto, this makes it hard to know at a glance if pointer lifetime is guaranteed. It seems like a foot-gun for RefPtr<Base> base = ...; auto derived = downcast<Derived>(base); to automatically remove the lifetime guarantee originally provided by base. You should have to do something special to remove lifetime guarantees. I think this means that downcast<T> should return a RefPtr<T> or Ref<T>, just like static_pointer_cast does. It's OK to optimize for the case of move. Then you end up with these options: (1) When you own the lifetime of base: auto derived = downcast<Derived>(base.get()); // This is like other uses of .get(), and it's my job to make sure it's OK just like other uses of .get() (2) When you transfer ownership of base: return downcast<Derived>(WTFMove(base)); // This returns RefPtr<T> to preserve lifetime without refcount churn (3) When you're not sure who owns the lifetime of base: auto derived = downcast<Derived>(base); // This increments the refcount, which is a safe default > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2369 > + return accessibilityImageMapHitTest(downcast<HTMLAreaElement>(WTFMove(node)).get(), point); WTFMove() followed by .get() doesn't make sense to me here. I guess this should just be .get() inside the downcast<>()? > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2372 > + node = downcast<HTMLOptionElement>(WTFMove(node))->ownerSelectElement(); I don't think WTFMove() is a great match here. We aren't trying to transfer ownership to anyone. We're just trying to call the ownerSelectElement() getter. We can do that with .get() or downcast<>() without moving anything. > Source/WebCore/css/StyleSheetContents.cpp:159 > + m_childRules.appendVector(downcast<StyleRule>(WTFMove(rule))->splitIntoMultipleRulesWithMaximumSelectorComponentCount(RuleData::maximumSelectorComponentCount)); I don't think you want WTFMove here. > Source/WebCore/css/parser/CSSParser.cpp:298 > + return downcast<StyleRuleFontFace>(WTFMove(rule))->properties().getPropertyCSSValue(propertyID); I don't think you want to move here. > Source/WebCore/dom/ContainerNode.cpp:740 > + downcast<ContainerNode>(*child).cloneChildNodes(downcast<ContainerNode>(WTFMove(clonedChild))); I don't think you want move here. > Source/WebCore/dom/Node.cpp:588 > + auto text = downcast<Text>(node.copyRef()); It feels very subtle that this line of code preserves a reference to the node. The use of auto means that we don't know the type of 'text' is RefPtr<T>. The use of downcast<T> is ambiguous because some variants of downcast<> return temporaries, whereas the particular variant for rvalue reference returns RefPtr<>. So, to know that this line of code preserves lifetime, you have to read the return type of copyRef() and then select the appropriate downcast<> for that return type, and then look at its return type, and then you know you have a RefPtr<>. That's a lot of code chasing just to understand pointer lifetime. But we want pointer lifetime to be known correct at a glance. > Source/WebCore/editing/ApplyStyleCommand.cpp:62 > + return is<CSSPrimitiveValue>(value) ? downcast<CSSPrimitiveValue>(WTFMove(value))->valueID() : 0; Move doesn't seem right here. > Source/WebCore/html/FormAssociatedElement.cpp:112 > + return downcast<HTMLFormElement>(WTFMove(newFormCandidate)).get(); I don't think you want move here. > Tools/TestWebKitAPI/Tests/WTF/RefPtr.cpp:566 > + EXPECT_NE((void*)p1.get(), (void*)p2); What I think you should do instead of void* casting is this: EXPECT_EQ(static_cast<Multi*>(p1.get()), p2); The nice thing about this is that it avoids relying on not-necessarily-obvious details of casting to void*. Also, it's usually better for a test to confirm that you got the one true correct answer, rather than confirming that you did not get one of many possible incorrect answers. > Tools/TestWebKitAPI/Tests/WTF/RefPtr.cpp:581 > + RefPtr<Base> p1 = adoptRef(new MultiReverse()); > + auto* p2 = downcast<MultiReverse>(p1); > + auto* p3 = downcast<MultiReverse>(p1.get()); > + EXPECT_EQ((void*)p1.get(), (void*)p2); > + EXPECT_EQ((void*)p1.get(), (void*)p3); > +} Same comment here.
Created attachment 325670 [details] Patch
Thanks Geoff for reviewing my patch. Here is the is<T>() only version patch per our discussion. (In reply to Geoffrey Garen from comment #66) > Comment on attachment 325605 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=325605&action=review > > The ability to call is<T> without a .get() looks great. > > This patch includes lots of uses of WTFMove that I don't think belong. I've > noted some but not all of them. I think you should fix that. We should only > WTFMove when we intend to transfer ownership, and not when we just want to > dereference an object and access one of its data members. > > I'm starting to think that it's bad for downcast<T> to implicitly remove the > RefPtr lifetime guarantee. In combination with auto, this makes it hard to > know at a glance if pointer lifetime is guaranteed. It seems like a foot-gun > for > > RefPtr<Base> base = ...; > auto derived = downcast<Derived>(base); > > to automatically remove the lifetime guarantee originally provided by base. > You should have to do something special to remove lifetime guarantees. > > I think this means that downcast<T> should return a RefPtr<T> or Ref<T>, > just like static_pointer_cast does. It's OK to optimize for the case of > move. Then you end up with these options: > > (1) When you own the lifetime of base: > > auto derived = downcast<Derived>(base.get()); // This is like other uses of > .get(), and it's my job to make sure it's OK just like other uses of .get() > > (2) When you transfer ownership of base: > > return downcast<Derived>(WTFMove(base)); // This returns RefPtr<T> to > preserve lifetime without refcount churn > > (3) When you're not sure who owns the lifetime of base: > > auto derived = downcast<Derived>(base); // This increments the refcount, > which is a safe default > > > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2369 > > + return accessibilityImageMapHitTest(downcast<HTMLAreaElement>(WTFMove(node)).get(), point); > > WTFMove() followed by .get() doesn't make sense to me here. I guess this > should just be .get() inside the downcast<>()? > > > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2372 > > + node = downcast<HTMLOptionElement>(WTFMove(node))->ownerSelectElement(); > > I don't think WTFMove() is a great match here. We aren't trying to transfer > ownership to anyone. We're just trying to call the ownerSelectElement() > getter. We can do that with .get() or downcast<>() without moving anything. > > > Source/WebCore/css/StyleSheetContents.cpp:159 > > + m_childRules.appendVector(downcast<StyleRule>(WTFMove(rule))->splitIntoMultipleRulesWithMaximumSelectorComponentCount(RuleData::maximumSelectorComponentCount)); > > I don't think you want WTFMove here. > > > Source/WebCore/css/parser/CSSParser.cpp:298 > > + return downcast<StyleRuleFontFace>(WTFMove(rule))->properties().getPropertyCSSValue(propertyID); > > I don't think you want to move here. > > > Source/WebCore/dom/ContainerNode.cpp:740 > > + downcast<ContainerNode>(*child).cloneChildNodes(downcast<ContainerNode>(WTFMove(clonedChild))); > > I don't think you want move here. > > > Source/WebCore/dom/Node.cpp:588 > > + auto text = downcast<Text>(node.copyRef()); > > It feels very subtle that this line of code preserves a reference to the > node. The use of auto means that we don't know the type of 'text' is > RefPtr<T>. The use of downcast<T> is ambiguous because some variants of > downcast<> return temporaries, whereas the particular variant for rvalue > reference returns RefPtr<>. So, to know that this line of code preserves > lifetime, you have to read the return type of copyRef() and then select the > appropriate downcast<> for that return type, and then look at its return > type, and then you know you have a RefPtr<>. That's a lot of code chasing > just to understand pointer lifetime. But we want pointer lifetime to be > known correct at a glance. > > > Source/WebCore/editing/ApplyStyleCommand.cpp:62 > > + return is<CSSPrimitiveValue>(value) ? downcast<CSSPrimitiveValue>(WTFMove(value))->valueID() : 0; > > Move doesn't seem right here. > > > Source/WebCore/html/FormAssociatedElement.cpp:112 > > + return downcast<HTMLFormElement>(WTFMove(newFormCandidate)).get(); > > I don't think you want move here. > > > Tools/TestWebKitAPI/Tests/WTF/RefPtr.cpp:566 > > + EXPECT_NE((void*)p1.get(), (void*)p2); > > What I think you should do instead of void* casting is this: > > EXPECT_EQ(static_cast<Multi*>(p1.get()), p2); > > The nice thing about this is that it avoids relying on > not-necessarily-obvious details of casting to void*. Also, it's usually > better for a test to confirm that you got the one true correct answer, > rather than confirming that you did not get one of many possible incorrect > answers. > > > Tools/TestWebKitAPI/Tests/WTF/RefPtr.cpp:581 > > + RefPtr<Base> p1 = adoptRef(new MultiReverse()); > > + auto* p2 = downcast<MultiReverse>(p1); > > + auto* p3 = downcast<MultiReverse>(p1.get()); > > + EXPECT_EQ((void*)p1.get(), (void*)p2); > > + EXPECT_EQ((void*)p1.get(), (void*)p3); > > +} > > Same comment here.
Thanks Ryosuke for r+ my patch.
Comment on attachment 325670 [details] Patch Clearing flags on attachment: 325670 Committed r224320: <https://trac.webkit.org/changeset/224320>
All reviewed patches have been landed. Closing bug.