Bug 120570 – Consider adding a reference-flavored smart pointer.

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Andreas Kling]

Consider adding a reference-flavored smart pointer.

[Andreas Kling]

Created attachment 210226 [details]
Patch idea

[Andreas Kling]

Created attachment 210227 [details]
Patch + various examples of use in WebCore

[Darin Adler]

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.

[Darin Adler]

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.

[Andreas Kling]

Comment on attachment 210226 [details]
Patch idea

Back to the drawin' board.

[Andreas Kling]

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.

[WebKit Commit Bot]

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.

[Antti Koivisto]

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.

[Andreas Kling]

(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.

[Antti Koivisto]

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?

[Darin Adler]

(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?

[Antti Koivisto]

> 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();
}

[Antti Koivisto]

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.

[Andreas Kling]

Committed r154962: <http://trac.webkit.org/changeset/154962>

[Csaba Osztrogonác]

(In reply to comment #14)
> Committed r154962: <http://trac.webkit.org/changeset/154962>
It broke the build everywhere,please land the new Ref.h

[Csaba Osztrogonác]

ping

[Andreas Kling]

Added Ref.h in <http://trac.webkit.org/changeset/154964>

[Darin Adler]

(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();
    }

[Antti Koivisto]

>     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.

[Darin Adler]

(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.

[Mikhail Pozdnyakov]

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)

[Darin Adler]

(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.