Bug 74421 – Cleanup of StringImpl::equal in r102631 post commit

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Michael Saboff]

The following comments were made to https://bugs.webkit.org/show_bug.cgi?id=73622 after it was committed:

 Comment #4 From Darin Adler 2011-12-13 08:54:19 PST (-) [reply] 
(From update of attachment 118843 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=118843&action=review

> Source/JavaScriptCore/wtf/text/AtomicString.h:142
> +inline bool operator==(const AtomicString& a, const Vector<UChar>& b) { return a.impl() && equal(static_cast<StringImpl*>(a.impl()), b.data(), b.size()); }    

Why is the cast needed?

> Source/JavaScriptCore/wtf/text/StringImpl.h:566
> +template <typename CharTypeL, typename CharTypeR>
> +    ALWAYS_INLINE bool equal(const CharTypeL*, const CharTypeR*, unsigned);

We don’t normally implement the second line like this.

I think that this is too broad an overload for equal. It will allow you to pass any two pointers, including pointers to that aren’t even character types. Instead, I suggest overloading the equal function for all the combinations.
 Comment #5 From Darin Adler 2011-12-13 08:54:38 PST (-) [reply] 
(In reply to comment #4)
> > Source/JavaScriptCore/wtf/text/StringImpl.h:566
> > +template <typename CharTypeL, typename CharTypeR>
> > +    ALWAYS_INLINE bool equal(const CharTypeL*, const CharTypeR*, unsigned);
> 
> We don’t normally indent the second line like this.

First of all, the cast in the operator==(AtomicString, ...) is needed because the template is too broad.  Changing equal to several overloaded functions will address the need for the cast.

There is another defect related to building WebKit on Mac Snow Leopard with gcc (https://bugs.webkit.org/show_bug.cgi?id=74387). Changing the equal() templates to overloaded functions might fix this as well.

[Michael Saboff]

Created attachment 119038 [details]
Patch

[Michael Saboff]

Committed r102692: <http://trac.webkit.org/changeset/102692>