Bug 74421

Summary: Cleanup of StringImpl::equal in r102631 post commit
Product: WebKit Reporter: Michael Saboff <msaboff>
Component: JavaScriptCoreAssignee: Michael Saboff <msaboff>
Status: RESOLVED FIXED    
Severity: Normal CC: darin
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch darin: review+

Description Michael Saboff 2011-12-13 10:45:42 PST
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.
Comment 1 Michael Saboff 2011-12-13 10:49:14 PST
Created attachment 119038 [details]
Patch
Comment 2 Michael Saboff 2011-12-13 11:39:26 PST
Committed r102692: <http://trac.webkit.org/changeset/102692>