[webkit-reviews] review denied: [Bug 39701] UTF-16 code points compare() for String objects : [Attachment 57137] patch 4

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Luiz Agostini
<luiz.agostini at openbossa.org>'s request for review:
Bug 39701: UTF-16 code points compare() for String objects
https://bugs.webkit.org/show_bug.cgi?id=39701

Attachment 57137: patch 4
https://bugs.webkit.org/attachment.cgi?id=57137&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
It’s fine to have a comparison function that just compares the UChars in a
string and does no case folding or normalization.

A) But I don't see any reason to make this comparison function be a class
member. Neither a static member nor a non-static member. It should just be a
free function, the way the function in UString.h already is. We can overload it
to take more types. This patch instead adds a member function to StringImpl,
but there's no reason for it to be a member function. It should just be a free
function.

I think codePointCompare is an OK name for the function. Better than just
"compare" since it might convince people not to use it in cases where they
shouldn’t.

B) We should rename the one that works on UString too, to be consistent. I
believe it's only called in exactly one place in JavaScriptCore.

C) The comparison function for UString that just calls through to the
codePointCompare that takes StringImpl* should be inlined. It's in a hot code
path and I don't want an extra level of function call added.

Now that I see the one place where codePointCompare is called inside
JavaScriptCore, I realize that it's a code path where the strings can never be
null strings. That has two implications:

    D) We don't have to treat null strings and empty strings as equal.
    E) If we wanted to we could make things slightly more efficient by making
that call site not even check for null.

review-, but I think you're getting close now to something we should land. You
should fix at least (A) and (C) above. And (B) is also a good idea and probably
easy to do. No real need to do (D) or (E), though.