Bug 150022 – Add helper function for checking pointer equivalency and use it

• Component: New Bugs
• Status: RESOLVED FIXED

[Simon Fraser (smfr)]

Add helper funtion for checking pointer equivalency and use it

[Simon Fraser (smfr)]

Created attachment 262877 [details]
Patch

[Simon Fraser (smfr)]

Not sure if RefPtr.h is the best place for bool areEquivalent(const T* a, const T* b), and it would be nice to have a unique_ptr version. Where to put it?

[Alexey Proskuryakov]

Comment on attachment 262877 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=262877&action=review

> Source/WTF/ChangeLog:3
> +        Add helper funtion for checking pointer equivalency and use it

Where does this use of the word "equivalency" originate from? I'm not familiar with it, and it looks quite misleading to me.

[Simon Fraser (smfr)]

It was used in the style code quite a bit:
-        && shadowDataEquivalent(o)
-        && willChangeDataEquivalent(o)
-        && reflectionDataEquivalent(o)
-        && animationDataEquivalent(o)
-        && transitionDataEquivalent(o)
+        && areEquivalent(m_counterDirectives.get(), o.m_counterDirectives.get())
+        && areEquivalent(m_boxShadow.get(), o.m_boxShadow.get())
+        && areEquivalent(m_willChange, o.m_willChange)
+        && areEquivalent(m_boxReflect, o.m_boxReflect)
+        && areEquivalent(m_animations.get(), o.m_animations.get())
+        && areEquivalent(m_transitions.get(), o.m_transitions.get())

[Darin Adler]

Comment on attachment 262877 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=262877&action=review

> Source/WTF/wtf/RefPtr.h:223
> +template<typename T> inline bool areEquivalent(const T* a, const T* b)
> +{ 
> +    return a == b || (a && b && *a == *b);
> +}

Should just implement this for all types instead of just pointers:

    template<typename T> inline bool areEquivalent(const T& a, const T& b)
    { 
        return a == b || (a && b && *a == *b);
    }

Then you don’t need a RefPtr overload. I suggest putting this somewhere in the WebCore tree instead of in WTF and naming it something more like isPointedToDataEqual.

[Simon Fraser (smfr)]

I'm not sure I can handle that singular use of data :)

[Simon Fraser (smfr)]

We like arePointingToEqualData().

[Simon Fraser (smfr)]

Created attachment 262971 [details]
Patch

[Simon Fraser (smfr)]

I put it in WTF because there's place in WebCore for shared, non-platform-specific utilities, and we may want to use it in JSC or WebKit.

[Simon Fraser (smfr)]

Created attachment 262973 [details]
Patch

[Darin Adler]

Comment on attachment 262973 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=262973&action=review

> Source/WebCore/rendering/style/FillLayer.cpp:157
> +    return arePointingToEqualData(m_image.get(), o.m_image.get()) && m_xPosition == o.m_xPosition && m_yPosition == o.m_yPosition

I believe there is no need for the .get() here since arePointingToEqualData works on smart pointers too, not just raw pointers.

> Source/WebCore/rendering/style/RenderStyle.cpp:559
> -        if (rareNonInheritedData->m_grid.get() != other.rareNonInheritedData->m_grid.get()
> -            || rareNonInheritedData->m_gridItem.get() != other.rareNonInheritedData->m_gridItem.get())
> +        if (rareNonInheritedData->m_grid != other.rareNonInheritedData->m_grid
> +            || rareNonInheritedData->m_gridItem != other.rareNonInheritedData->m_gridItem)

Too bad we don’t have test cases for this bug fix.

> Source/WebCore/rendering/style/RenderStyle.cpp:943
> +    if (arePointingToEqualData(rareNonInheritedData->m_willChange.get(), willChangeData.get()))

I believe there is no need for the .get() here since arePointingToEqualData works on smart pointers too, not just raw pointers.

[Simon Fraser (smfr)]

(In reply to comment #11)
> Comment on attachment 262973 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=262973&action=review
> 
> > Source/WebCore/rendering/style/FillLayer.cpp:157
> > +    return arePointingToEqualData(m_image.get(), o.m_image.get()) && m_xPosition == o.m_xPosition && m_yPosition == o.m_yPosition
> 
> I believe there is no need for the .get() here since arePointingToEqualData
> works on smart pointers too, not just raw pointers.

I'll fix.

> > Source/WebCore/rendering/style/RenderStyle.cpp:559
> > -        if (rareNonInheritedData->m_grid.get() != other.rareNonInheritedData->m_grid.get()
> > -            || rareNonInheritedData->m_gridItem.get() != other.rareNonInheritedData->m_gridItem.get())
> > +        if (rareNonInheritedData->m_grid != other.rareNonInheritedData->m_grid
> > +            || rareNonInheritedData->m_gridItem != other.rareNonInheritedData->m_gridItem)
> 
> Too bad we don’t have test cases for this bug fix.

Yes!

> > Source/WebCore/rendering/style/RenderStyle.cpp:943
> > +    if (arePointingToEqualData(rareNonInheritedData->m_willChange.get(), willChangeData.get()))
> 
> I believe there is no need for the .get() here since arePointingToEqualData
> works on smart pointers too, not just raw pointers.

These are different types (PassRefPtr vs RefPtr), so the .get() are needed to turn both into raw pointers.

[Simon Fraser (smfr)]

https://trac.webkit.org/r191017