[Webkit-unassigned] [Bug 42726] Refactor CSSSelector's destructor and make it inline.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=42726





--- Comment #4 from Darin Adler <darin at apple.com>  2010-07-21 08:27:31 PST ---
(From update of attachment 62155)
This patch is great! Thanks for taking my suggestions.

I am not wholly happy with the use of the term "children" to mean the objects a selector owns. It's standard to do such a thing in a generic tree structure, but in this case neither "tag history" nor "simple selector" seems to be entirely like a child. But since I don't have better terminology to suggest, I guess you can stick with "children" unless you can think of something better.

> +inline void CSSSelector::releaseChildrenToBag(CSSSelector* selector, CSSSelectorBag* bag)

This function could, and should, take a reference to the bag instead of a pointer.

I don't see why this is a static member function that takes a CSSSelector* rather than a non-static member function. It would be easier to read as a normal member function.

> +        ~CSSSelector()
> +        {
> +            // We should avoid a recursive destructor call, which causes stack overflow
> +            // if selectors are deeply nested.
> +            if (m_hasRareData) {
> +                if (!m_data.m_rareData)
> +                    return;
> +            } else if (!m_data.m_tagHistory)
> +                return;
> +            deleteChildren();
> +        }

I find this comment confusing. The comment explains why we call deleteChildren instead of just calling delete directly on the objects we own, but readers of the comment probably can't figure that out. Also, the comment is above the if statement, which has a different explanation: It's optimizing the common case where the selector does not own any pointers it needs to delete.

Since this is a refactoring patch, review- so you can fix the comment and the static member function.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.