Bug 120570 - Consider adding a reference-flavored smart pointer.
Summary: Consider adding a reference-flavored smart pointer.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andreas Kling
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-08-31 22:13 PDT by Andreas Kling
Modified: 2013-09-06 12:38 PDT (History)
10 users (show)

See Also:


Attachments
Patch idea (11.52 KB, patch)
2013-08-31 22:16 PDT, Andreas Kling
no flags Details | Formatted Diff | Diff
Patch + various examples of use in WebCore (49.18 KB, patch)
2013-08-31 22:18 PDT, Andreas Kling
no flags Details | Formatted Diff | Diff
Simplified version for stack guarding (85.36 KB, patch)
2013-09-01 07:48 PDT, Andreas Kling
koivisto: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andreas Kling 2013-08-31 22:13:11 PDT
Consider adding a reference-flavored smart pointer.
Comment 1 Andreas Kling 2013-08-31 22:16:19 PDT
Created attachment 210226 [details]
Patch idea
Comment 2 Andreas Kling 2013-08-31 22:18:58 PDT
Created attachment 210227 [details]
Patch + various examples of use in WebCore
Comment 3 Darin Adler 2013-08-31 22:54:29 PDT
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 4 Darin Adler 2013-08-31 22:55:23 PDT
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 5 Andreas Kling 2013-08-31 23:27:49 PDT
Comment on attachment 210226 [details]
Patch idea

Back to the drawin' board.
Comment 6 Andreas Kling 2013-09-01 07:48:22 PDT
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.
Comment 7 WebKit Commit Bot 2013-09-01 07:50:03 PDT
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 8 Antti Koivisto 2013-09-01 09:25:53 PDT
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.
Comment 9 Andreas Kling 2013-09-01 09:31:05 PDT
(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 10 Antti Koivisto 2013-09-01 09:48:44 PDT
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?
Comment 11 Darin Adler 2013-09-01 12:12:19 PDT
(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?
Comment 12 Antti Koivisto 2013-09-02 10:51:08 PDT
> 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 13 Antti Koivisto 2013-09-02 11:06:20 PDT
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.
Comment 14 Andreas Kling 2013-09-02 11:49:09 PDT
Committed r154962: <http://trac.webkit.org/changeset/154962>
Comment 15 Csaba Osztrogonác 2013-09-02 12:10:42 PDT
(In reply to comment #14)
> Committed r154962: <http://trac.webkit.org/changeset/154962>
It broke the build everywhere,please land the new Ref.h
Comment 16 Csaba Osztrogonác 2013-09-02 12:40:15 PDT
ping
Comment 17 Andreas Kling 2013-09-02 13:08:40 PDT
Added Ref.h in <http://trac.webkit.org/changeset/154964>
Comment 18 Darin Adler 2013-09-02 22:50:50 PDT
(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();
    }
Comment 19 Antti Koivisto 2013-09-03 01:20:45 PDT
>     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.
Comment 20 Darin Adler 2013-09-03 10:03:37 PDT
(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.
Comment 21 Mikhail Pozdnyakov 2013-09-06 07:26:20 PDT
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)
Comment 22 Darin Adler 2013-09-06 12:38:01 PDT
(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.