Bug 188539 – Make CSSSelectorList a little more sane

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Alex Christensen]

Make CSSSelectorList a little more sane

[Alex Christensen]

Created attachment 347051 [details]
Patch

[Alex Christensen]

http://trac.webkit.org/r234825

[Radar WebKit Bug Importer]

<rdar://problem/43258896>

[Darin Adler]

How is UniqueArray<T> different from std::unique_ptr<T[]>? I’m surprised we added our own template for this instead of using the standard library.

[Darin Adler]

Comment on attachment 347051 [details]
Patch

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

> Source/WebCore/css/CSSSelectorList.cpp:48
>  CSSSelectorList::CSSSelectorList(CSSSelectorList&& other)
> -    : m_selectorArray(other.m_selectorArray)
> +    : m_selectorArray(WTFMove(other.m_selectorArray))
>  {
> -    other.m_selectorArray = nullptr;
>  }

This now seems to be doing what the default move constructor would do. Can we change this so we don’t write it out explicitly?

> Source/WebCore/css/CSSSelectorList.cpp:113
>  CSSSelectorList& CSSSelectorList::operator=(CSSSelectorList&& other)
>  {
> -    deleteSelectors();
> -    m_selectorArray = other.m_selectorArray;
> -    other.m_selectorArray = nullptr;
> -
> +    m_selectorArray = WTFMove(other.m_selectorArray);
>      return *this;
>  }

This now seems to be doing what the default move assignment operator would do. Can we change this so we don’t write it out explicitly?

> Source/WebCore/css/CSSSelectorList.cpp:-131
> -    bool isLastSelector = false;
> -    for (CSSSelector* s = selectorArray; !isLastSelector; ++s) {
> -        isLastSelector = s->isLastInSelectorList();
> -        s->~CSSSelector();
> -    }

Does the UniqueArray have this same optimization? Why isn’t this check for isLastInSelectorList needed any more?

> Source/WebCore/css/CSSSelectorList.h:42
>      CSSSelectorList(Vector<std::unique_ptr<CSSParserSelector>>&&);

I think this maybe this should be marked "explicit".

> Source/WebCore/css/CSSSelectorList.h:44
> +    CSSSelectorList(UniqueArray<CSSSelector>&& array)
> +        : m_selectorArray(WTFMove(array)) { }

I think this maybe this should be marked "explicit".

> Source/WebCore/css/CSSSelectorList.h:75
>      // End of the array is indicated by m_isLastInSelectorList bit in the last item.

This comment is now confusing to me. Apparently that’s not how the end of the array works any more? Or is it?

[Alex Christensen]

(In reply to Darin Adler from comment #5)
> > Source/WebCore/css/CSSSelectorList.cpp:-131
> > -    bool isLastSelector = false;
> > -    for (CSSSelector* s = selectorArray; !isLastSelector; ++s) {
> > -        isLastSelector = s->isLastInSelectorList();
> > -        s->~CSSSelector();
> > -    }
> 
> Does the UniqueArray have this same optimization? Why isn’t this check for
> isLastInSelectorList needed any more?
UniqueArray, like std::unique_ptr<CSSSelector[]>, uses malloc information to know how many destructors to call.

> > Source/WebCore/css/CSSSelectorList.h:75
> >      // End of the array is indicated by m_isLastInSelectorList bit in the last item.
> 
> This comment is now confusing to me. Apparently that’s not how the end of
> the array works any more? Or is it?
It's still how the end of the array works for iterating because we don't have std::dynarray.  I was considering making it a Vector, but that would use extra bits and I didn't want to change behavior or performance in this patch.  Do you think it would be good to make it a Vector instead of a UniqueArray?

[Alex Christensen]

(In reply to Darin Adler from comment #4)
> How is UniqueArray<T> different from std::unique_ptr<T[]>? I’m surprised we
> added our own template for this instead of using the standard library.

UniqueArray uses fastMalloc and fastFree.  It's nicer than adding a bunch of template parameters everywhere.

[Darin Adler]

(In reply to Alex Christensen from comment #6

> Do you think it would be good to make it a Vector instead of a
> UniqueArray?

No.

[Darin Adler]

(In reply to Alex Christensen from comment #7)
> (In reply to Darin Adler from comment #4)
> > How is UniqueArray<T> different from std::unique_ptr<T[]>? I’m surprised we
> > added our own template for this instead of using the standard library.
> 
> UniqueArray uses fastMalloc and fastFree.  It's nicer than adding a bunch of
> template parameters everywhere.

Why doesn’t this happen for std::unique_ptr?

[Alex Christensen]

(In reply to Darin Adler from comment #9)
> (In reply to Alex Christensen from comment #7)
> > (In reply to Darin Adler from comment #4)
> > > How is UniqueArray<T> different from std::unique_ptr<T[]>? I’m surprised we
> > > added our own template for this instead of using the standard library.
> > 
> > UniqueArray uses fastMalloc and fastFree.  It's nicer than adding a bunch of
> > template parameters everywhere.
> 
> Why doesn’t this happen for std::unique_ptr?

It can, but it requires a second template parameter.  That's how we implemented UniqueArray.  make_unique also can't be used with fastMalloc, so we made makeUniqueArray.

[Darin Adler]

Why aren’t we using a WTF::UniquePtr too for the same reason?

[Alex Christensen]

We actually could've for this case because WTF_MAKE_FAST_ALLOCATED makes operator new[] and operator delete[], but if we use types with no such operators (such as char) we need something like UniqueArray.  I'm using UniqueArray for consistency with those cases.  See https://bugs.webkit.org/show_bug.cgi?id=182975

[Alex Christensen]

Responding to the remaining feedback in https://bugs.webkit.org/show_bug.cgi?id=188566