Summary: | Consider adding a reference-flavored smart pointer. | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Andreas Kling <kling> | ||||||||
Component: | Web Template Framework | Assignee: | Andreas Kling <kling> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | benjamin, cmarcelo, commit-queue, darin, gyuyoung.kim, kling, koivisto, mikhail.pozdnyakov, ossy, rakuco | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Andreas Kling
2013-08-31 22:13:11 PDT
Created attachment 210226 [details]
Patch idea
Created attachment 210227 [details]
Patch + various examples of use in WebCore
Comment on attachment 210226 [details] Patch idea View in context: https://bugs.webkit.org/attachment.cgi?id=210226&action=review We might want a move constructor for this class. We can’t make one of these null, because that will invalidate its invariant, but perhaps we can still do a move when the other Ref is falling out of scope? Anders would know for sure. > Source/WTF/wtf/Ref.h:38 > + Ref(const T&); This should not be needed. If we want to do a Ref for something that's const, the const can be part of the T. > Source/WTF/wtf/Ref.h:41 > + template<typename U> Ref(U&); Given this function template, it’s not clear we need Ref(T&) at all. We should try just leaving it out. > Source/WTF/wtf/Ref.h:43 > + // These two will incur the cost of a null-check to guarantee that Ref always points at something. Is the CRASH() really what we want here? I worry that we’ll be seeing this way too much. Why not require that people use a * and just leave this “convenience” out entirely? What’s so special about RefPtr and PassRefPtr? Why not also do this for raw pointers? > Source/WTF/wtf/Ref.h:49 > + T& get() const { return *m_ptr; } So you always have to say get(). when using one of these? That’s kinda ugly. Uses of RefPtr look good because of ->, but I can see how that’s not sensible here, so maybe we just have to live with this. Wish there was something better than get(). > Source/WTF/wtf/Ref.h:51 > + void swap(Ref&); You can’t swap a real reference, so why do we need to swap these? Put another way, if we can’t assign these I see no reason we need to swap them. > Source/WTF/wtf/Ref.h:53 > + static T* hashTableDeletedValue() { return reinterpret_cast<T*>(-1); } Not sure this is helpful on its own. We also need a hash table empty value, and since this has no null, no idea what that would be. Leave this out unless you have a solution to that problem. > Source/WTF/wtf/Ref.h:56 > + Ref& operator=(const Ref<T>&); Why is this private? Just because real references can’t be assigned? If private, then I think we want to delete it too. > Source/WTF/wtf/Ref.h:77 > + : m_ptr(&o.get()) I think this should be o.m_ptr, not &o.get(). > Source/WTF/wtf/Ref.h:84 > + : m_ptr(&o.get()) Hmm, maybe this shows why it needs to be &o.get(), since we can’t get at o’s private members. Comment on attachment 210226 [details] Patch idea View in context: https://bugs.webkit.org/attachment.cgi?id=210226&action=review > Source/WTF/wtf/Ref.h:123 > +template<typename T, typename U> inline bool operator==(const Ref<T>& a, const Ref<U>& b) These == and != operators don’t seem right. You can’t compare real references, so why do we need to compare these? If we’re not doing assignment I don’t think we need to do comparison either. Comment on attachment 210226 [details]
Patch idea
Back to the drawin' board.
Created attachment 210241 [details]
Simplified version for stack guarding
Okay, here's how I think we can start: let's just support the stack guard use case at first.
Then we can add more capabilities as we go along.
Attachment 210241 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/WTF.pro', u'Source/WTF/WTF.vcxproj/WTF.vcxproj.filters', u'Source/WTF/WTF.xcodeproj/project.pbxproj', u'Source/WTF/wtf/CMakeLists.txt', u'Source/WTF/wtf/Forward.h', u'Source/WTF/wtf/Ref.h', u'Source/WebCore/ChangeLog', u'Source/WebCore/Modules/geolocation/Geolocation.cpp', u'Source/WebCore/Modules/webaudio/AudioContext.cpp', u'Source/WebCore/bindings/js/JSCustomSQLStatementErrorCallback.cpp', u'Source/WebCore/bindings/js/JSCustomXPathNSResolver.cpp', u'Source/WebCore/bindings/js/JSDOMGlobalObjectTask.cpp', u'Source/WebCore/bindings/js/JSErrorHandler.cpp', u'Source/WebCore/bindings/js/JSEventListener.cpp', u'Source/WebCore/bindings/js/JSEventListener.h', u'Source/WebCore/bridge/runtime_root.cpp', u'Source/WebCore/css/CSSFontSelector.cpp', u'Source/WebCore/css/StyleSheetContents.cpp', u'Source/WebCore/dom/CharacterData.cpp', u'Source/WebCore/dom/ContainerNode.cpp', u'Source/WebCore/dom/ContainerNodeAlgorithms.h', u'Source/WebCore/dom/Document.cpp', u'Source/WebCore/dom/DocumentEventQueue.cpp', u'Source/WebCore/dom/EventTarget.cpp', u'Source/WebCore/dom/Node.cpp', u'Source/WebCore/dom/ScriptExecutionContext.cpp', u'Source/WebCore/dom/ScriptedAnimationController.cpp', u'Source/WebCore/html/HTMLEmbedElement.cpp', u'Source/WebCore/html/HTMLFormControlElement.cpp', u'Source/WebCore/html/HTMLFormElement.cpp', u'Source/WebCore/html/HTMLFrameOwnerElement.cpp', u'Source/WebCore/html/HTMLInputElement.cpp', u'Source/WebCore/html/HTMLLinkElement.cpp', u'Source/WebCore/html/HTMLMediaElement.cpp', u'Source/WebCore/html/HTMLObjectElement.cpp', u'Source/WebCore/html/HTMLOptionElement.cpp', u'Source/WebCore/html/HTMLScriptElement.cpp', u'Source/WebCore/html/HTMLTableElement.cpp', u'Source/WebCore/html/HTMLTextAreaElement.cpp', u'Source/WebCore/html/HTMLTitleElement.cpp', u'Source/WebCore/html/parser/HTMLDocumentParser.cpp', u'Source/WebCore/html/shadow/SpinButtonElement.cpp', u'Source/WebCore/html/shadow/TextControlInnerElements.cpp', u'Source/WebCore/inspector/InspectorCSSAgent.cpp', u'Source/WebCore/loader/DocumentLoader.cpp', u'Source/WebCore/loader/DocumentThreadableLoader.cpp', u'Source/WebCore/loader/DocumentWriter.cpp', u'Source/WebCore/loader/FrameLoader.cpp', u'Source/WebCore/loader/NavigationScheduler.cpp', u'Source/WebCore/loader/NetscapePlugInStreamLoader.cpp', u'Source/WebCore/loader/ResourceLoader.cpp', u'Source/WebCore/loader/SubresourceLoader.cpp', u'Source/WebCore/loader/cf/SubresourceLoaderCF.cpp', u'Source/WebCore/page/DOMWindow.cpp', u'Source/WebCore/page/DOMWindowExtension.cpp', u'Source/WebCore/page/FocusController.cpp', u'Source/WebCore/page/FrameView.cpp', u'Source/WebCore/page/animation/AnimationBase.cpp', u'Source/WebCore/platform/graphics/DisplayRefreshMonitor.cpp', u'Source/WebCore/platform/mac/WidgetMac.mm', u'Source/WebCore/platform/network/BlobResourceHandle.cpp', u'Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp', u'Source/WebCore/platform/network/mac/ResourceHandleMac.mm', u'Source/WebCore/rendering/RenderWidget.cpp', u'Source/WebCore/workers/WorkerScriptLoader.cpp', u'Source/WebCore/xml/XMLHttpRequest.cpp', u'Source/WebCore/xml/parser/XMLDocumentParser.cpp', u'Source/WebCore/xml/parser/XMLDocumentParserLibxml2.cpp']" exit_code: 1
Source/WebCore/rendering/RenderWidget.cpp:36: Alphabetical sorting problem. [build/include_order] [4]
Source/WebCore/xml/parser/XMLDocumentParserLibxml2.cpp:65: Alphabetical sorting problem. [build/include_order] [4]
Total errors found: 2 in 69 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 210241 [details] Simplified version for stack guarding View in context: https://bugs.webkit.org/attachment.cgi?id=210241&action=review > Source/WTF/wtf/Ref.h:38 > + ALWAYS_INLINE T& get() const { return *m_ptr; } How about having operator* and operator-> instead and sticking with pointer-like semantics? Getting reference-like semantics right seem impossible since there is no operator.() etc. (In reply to comment #8) > (From update of attachment 210241 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=210241&action=review > > > Source/WTF/wtf/Ref.h:38 > > + ALWAYS_INLINE T& get() const { return *m_ptr; } > > How about having operator* and operator-> instead and sticking with pointer-like semantics? Getting reference-like semantics right seem impossible since there is no operator.() etc. * and -> make me nervous now, I don't want to go back. :| I agree that .get() is srsly ugly. I originally thought that we could reduce aesthetic damage by using accessors instead of poking at the Ref directly. Maybe it's not so bad. Let me just go through the 7 stages. Comment on attachment 210241 [details] Simplified version for stack guarding View in context: https://bugs.webkit.org/attachment.cgi?id=210241&action=review >>> Source/WTF/wtf/Ref.h:38 >>> + ALWAYS_INLINE T& get() const { return *m_ptr; } >> >> How about having operator* and operator-> instead and sticking with pointer-like semantics? Getting reference-like semantics right seem impossible since there is no operator.() etc. > > * and -> make me nervous now, I don't want to go back. :| > > I agree that .get() is srsly ugly. I originally thought that we could reduce aesthetic damage by using accessors instead of poking at the Ref directly. > > Maybe it's not so bad. Let me just go through the 7 stages. Could we also get correct const? (In reply to comment #10) > (From update of attachment 210241 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=210241&action=review > > >>> Source/WTF/wtf/Ref.h:38 > >>> + ALWAYS_INLINE T& get() const { return *m_ptr; } > >> > >> How about having operator* and operator-> instead and sticking with pointer-like semantics? Getting reference-like semantics right seem impossible since there is no operator.() etc. > > > > * and -> make me nervous now, I don't want to go back. :| > > > > I agree that .get() is srsly ugly. I originally thought that we could reduce aesthetic damage by using accessors instead of poking at the Ref directly. > > > > Maybe it's not so bad. Let me just go through the 7 stages. > > Could we also get correct const? I didn’t see incorrect const here. A Ref that itself is const does not mean the referred-to object is const. The const for the referred-to object would be inside the T type. Or maybe you were talking about something else? > I didn’t see incorrect const here. A Ref that itself is const does not mean the referred-to object is const. The const for the referred-to object would be inside the T type.
>
> Or maybe you were talking about something else?
struct Foo {
void changeInternalStateFromConstFunction() const;
Ref<Bar> m_bar;
}
voi Foo::changeInternalStateFromConstFunction() const
{
// Implicit const_cast
Bar& constFreeBar = m_bar.get();
constFreeBar.goCrazy();
}
Comment on attachment 210241 [details]
Simplified version for stack guarding
I think i'm fine with .get(). Please fix the constness with separate const and non-const versions. I know RefPtr does this but it is equally wrong there too.
Committed r154962: <http://trac.webkit.org/changeset/154962> (In reply to comment #14) > Committed r154962: <http://trac.webkit.org/changeset/154962> It broke the build everywhere,please land the new Ref.h ping Added Ref.h in <http://trac.webkit.org/changeset/154964> (In reply to comment #12) > > I didn’t see incorrect const here. A Ref that itself is const does not mean the referred-to object is const. The const for the referred-to object would be inside the T type. > > > > Or maybe you were talking about something else? > > struct Foo { > void changeInternalStateFromConstFunction() const; > Ref<Bar> m_bar; > } > > voi Foo::changeInternalStateFromConstFunction() const > { > // Implicit const_cast > Bar& constFreeBar = m_bar.get(); > constFreeBar.goCrazy(); > } Exactly, that’s just like a reference: struct Foo { void changeInternalStateFromConstFunction() const; Bar& m_bar; } void Foo::changeInternalStateFromConstFunction() const { // m_bar is not part of Foo, it's a separate object, so the const-ness of Foo is not relevant Bar& constFreeBar = m_bar; constFreeBar.goCrazy(); } Or a pointer: struct Foo { void changeInternalStateFromConstFunction() const; Bar* m_bar; } void Foo::changeInternalStateFromConstFunction() const { // m_bar is not part of Foo, it's a separate object, so the const-ness of Foo is not relevant Bar& constFreeBar = *m_bar; constFreeBar.goCrazy(); } > void Foo::changeInternalStateFromConstFunction() const
> {
> // m_bar is not part of Foo, it's a separate object, so the const-ness of Foo is not relevant
> Bar& constFreeBar = *m_bar;
> constFreeBar.goCrazy();
> }
I think it might be appropriate for Refs to enforce stronger constness than raw references/pointers. They imply and implement (shared) ownership. An owned object can be considered part of the containers state.
(In reply to comment #19) > I think it might be appropriate for Refs to enforce stronger constness than raw references/pointers. OK. While it’s not traditional for smart pointers to do this type of thing, I’m OK with it, then. As far as I see Ref is a an analogue of RefPtr but it enforces that m_ptr!=0. My question is: should not we introduce PassRef class as well? So far the API is supposed to accept/return references to the object, however it's quite unclear for the client whether the obtained reference is from RefCounted object pointer and hence whether Ref class should be used for storing it. Introducing of RefPass would solve the problem. (Another solution is to provide move semantics for Ref and pass Ref instances) (In reply to comment #21) > My question is: should not we introduce PassRef class as well? We are probably going to be able to get rid of PassRefPtr and use RefPtr instead soon, taking advantage of C++11 move semantics instead of our own home-grown equivalent. If we find this is not going to happen, then we can add a PassRef, but if it does, we should just start using Ref this new way instead of adding PassRef. |