Closed
Bug 1455559
Opened 6 years ago
Closed 6 years ago
FromNode should work for references.
Categories
(Core :: DOM: Core & HTML, enhancement)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: emilio, Assigned: emilio)
References
Details
Attachments
(1 file)
Right now you need to use: Foo::FromNode(&aRef) Which is quite ugly.
Comment hidden (mozreview-request) |
Updated•6 years ago
|
Component: DOM: Animation → DOM
Comment 2•6 years ago
|
||
mozreview-review |
Comment on attachment 8969585 [details] Bug 1455559: Make FromNode work for references. https://reviewboard.mozilla.org/r/238348/#review244288 This is fine. I'm not convinced as to how useful having a reference overload is, but I don't personally mind it. ::: dom/base/nsINode.h:2169 (Diff revision 1) > - static const _class* FromNode(const ArgType* aNode) \ > - { \ > - return aNode->_check ? static_cast<const _class*>(aNode) : nullptr; \ > - } \ > - template<typename ArgType> \ > - static const _class* FromNodeOrNull(const ArgType* aNode) \ > - { \ > - return aNode ? FromNode(aNode) : nullptr; \ > + template<typename T, template<typename> typename SmartPtr> \ > + static _const _class* FromNode(_const SmartPtr<T>& aNode) \ > + { \ > + return FromNode(aNode.get()); \ > + } \ > + template<typename T, template<typename> typename SmartPtr> \ > + static _const _class* FromNodeOrNull(_const SmartPtr<T>& aNode) \ > + { \ > + return FromNodeOrNull(aNode.get()); \ > } I _think_ you can get rid of these two overloads if you change the `_const T&` version to instead be: static auto FromNode(_const T& aNode) -> decltype(static_cast<_const _class*>(&aNode)) which uses SFINAE to check if the cast would succeed before selecting the method during overload resolution.
Attachment #8969585 -
Flags: review?(nika) → review+
Comment 3•6 years ago
|
||
mozreview-review |
Comment on attachment 8969585 [details] Bug 1455559: Make FromNode work for references. https://reviewboard.mozilla.org/r/238348/#review244380 ::: dom/base/nsINode.h:2175 (Diff revision 1) > - { \ > - return aNode->_check ? static_cast<const _class*>(aNode) : nullptr; \ > - } \ > - template<typename ArgType> \ > - static const _class* FromNodeOrNull(const ArgType* aNode) \ > - { \ > + static _const _class* FromNode(_const SmartPtr<T>& aNode) \ > + { \ > + return FromNode(aNode.get()); \ > + } \ > + template<typename T, template<typename> typename SmartPtr> \ > + static _const _class* FromNodeOrNull(_const SmartPtr<T>& aNode) \ This overload doesn't quite makes sense to me. A const smartptr returns a pointer to a non-const thing from get(), so there's no real reason to have `_const` in the args for the smartptr overloads.
Attachment #8969585 -
Flags: review?(bzbarsky) → review+
Comment hidden (mozreview-request) |
Assignee | ||
Comment 5•6 years ago
|
||
Comment on attachment 8969585 [details] Bug 1455559: Make FromNode work for references. I ended up doing something in between because our compilers on automation didn't like template template parameters, could do a once-over :)
Attachment #8969585 -
Flags: review+ → review?(bzbarsky)
Assignee | ||
Comment 6•6 years ago
|
||
mozreview-review |
Comment on attachment 8969585 [details] Bug 1455559: Make FromNode work for references. https://reviewboard.mozilla.org/r/238348/#review244436 ::: dom/base/nsINode.h:2169 (Diff revision 1) > - static const _class* FromNode(const ArgType* aNode) \ > - { \ > - return aNode->_check ? static_cast<const _class*>(aNode) : nullptr; \ > - } \ > - template<typename ArgType> \ > - static const _class* FromNodeOrNull(const ArgType* aNode) \ > - { \ > - return aNode ? FromNode(aNode) : nullptr; \ > + template<typename T, template<typename> typename SmartPtr> \ > + static _const _class* FromNode(_const SmartPtr<T>& aNode) \ > + { \ > + return FromNode(aNode.get()); \ > + } \ > + template<typename T, template<typename> typename SmartPtr> \ > + static _const _class* FromNodeOrNull(_const SmartPtr<T>& aNode) \ > + { \ > + return FromNodeOrNull(aNode.get()); \ > } Nice trick! I ended up using it :) ::: dom/base/nsINode.h:2175 (Diff revision 1) > - { \ > - return aNode->_check ? static_cast<const _class*>(aNode) : nullptr; \ > - } \ > - template<typename ArgType> \ > - static const _class* FromNodeOrNull(const ArgType* aNode) \ > - { \ > + static _const _class* FromNode(_const SmartPtr<T>& aNode) \ > + { \ > + return FromNode(aNode.get()); \ > + } \ > + template<typename T, template<typename> typename SmartPtr> \ > + static _const _class* FromNodeOrNull(_const SmartPtr<T>& aNode) \ You're right, it should read `const SmartPtr<_const T>&` I guess. Though it's gone in the next version. I guess we don't use those much in DOM Code anyway.
Comment 7•6 years ago
|
||
mozreview-review |
Comment on attachment 8969585 [details] Bug 1455559: Make FromNode work for references. https://reviewboard.mozilla.org/r/238348/#review245466
Attachment #8969585 -
Flags: review?(bzbarsky) → review+
Pushed by ecoal95@gmail.com: https://hg.mozilla.org/integration/autoland/rev/ad5095a897e5 Make FromNode work for references. r=bz,Nika
Comment 9•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ad5095a897e5
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•