Bug 74421 - Cleanup of StringImpl::equal in r102631 post commit
Summary: Cleanup of StringImpl::equal in r102631 post commit
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Michael Saboff
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-12-13 10:45 PST by Michael Saboff
Modified: 2011-12-13 11:39 PST (History)
1 user (show)

See Also:


Attachments
Patch (7.08 KB, patch)
2011-12-13 10:49 PST, Michael Saboff
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>