<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body><span class="vcard"><a class="email" href="mailto:darin@apple.com" title="Darin Adler <darin@apple.com>"> <span class="fn">Darin Adler</span></a>
</span> changed
<a class="bz_bug_link
bz_status_NEW "
title="NEW - [INTL] Implement Collator Compare Functions"
href="https://bugs.webkit.org/show_bug.cgi?id=147604">bug 147604</a>
<br>
<table border="1" cellspacing="0" cellpadding="8">
<tr>
<th>What</th>
<th>Removed</th>
<th>Added</th>
</tr>
<tr>
<td style="text-align:right;">Attachment #263825 Flags</td>
<td>review?
</td>
<td>review+
</td>
</tr></table>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - [INTL] Implement Collator Compare Functions"
href="https://bugs.webkit.org/show_bug.cgi?id=147604#c9">Comment # 9</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - [INTL] Implement Collator Compare Functions"
href="https://bugs.webkit.org/show_bug.cgi?id=147604">bug 147604</a>
from <span class="vcard"><a class="email" href="mailto:darin@apple.com" title="Darin Adler <darin@apple.com>"> <span class="fn">Darin Adler</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=263825&action=diff" name="attach_263825" title="Patch">attachment 263825</a> <a href="attachment.cgi?id=263825&action=edit" title="Patch">[details]</a></span>
Patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=263825&action=review">https://bugs.webkit.org/attachment.cgi?id=263825&action=review</a>
Looks OK. I see a lot of room for refinement and improvement.
<span class="quote">> Source/JavaScriptCore/runtime/IntlCollator.h:61
> + bool initializedCollator() const { return m_initializedCollator; }</span >
The naming here is unfortunate. This sounds like a function that returns a collator that has been initialized.
<span class="quote">> Source/JavaScriptCore/runtime/IntlCollator.h:62
> + void setInitializedCollator(bool initializedCollator) { m_initializedCollator = initializedCollator; }</span >
The naming here is unfortunate. This sounds like a function that takes a collator as an argument, not a function that takes a boolean.
Using public functions for this is taking the standards language too literally. We should keep this boolean private and write a member function that initializes if needed and returns if not.
<span class="quote">> Source/JavaScriptCore/runtime/IntlCollatorConstructor.h:63
> +IntlCollator* initializeCollator(ExecState*, IntlCollator*, JSValue locales, JSValue optionsValue);</span >
The interface to this function is strange. Why does the function both take a collator as an argument and return one. If the function needs to indicate failure, I suggest it return a boolean or error code. The ExecState argument should be ExecState&, not ExecState*, and the collator argument should be IntlCollator&, not IntlCollator*.
<span class="quote">> Source/JavaScriptCore/runtime/IntlCollatorPrototype.cpp:90
> +static int compareStrings(IntlCollator* collator, const String& x, const String& y)</span >
This should take an IntlCollator&, not an IntlCollator*.
Also should probably take two StringView arguments, not two const String& arguments.
We’re going to need performance tests for this. Right now this is super-slow and we want a version that has reasonably fast performance.
<span class="quote">> Source/JavaScriptCore/runtime/IntlCollatorPrototype.cpp:94
> + UCollator* icuCollator = ucol_open(collator->locale().utf8().data(), &status);</span >
For acceptable performance, I think it’s important that we not use ucol_open and ucol_close every time. I tried that in code in WebCore and found that everything was much too slow. I think we probably need to put the UCollator in the IntlCollator, not create a new one every time.
Doing all that work to initialize the collator attributes every time we compare a pair of strings is going to result in very poor performance.
We also need to come up with a fast path so we aren’t converting simple strings with all ASCII characters to 16 bit just to call ICU with them. Again, we’ve run into this in the past. We need performance tests for this.
<span class="quote">> Source/JavaScriptCore/runtime/IntlCollatorPrototype.cpp:96
> + return codePointCompare(x, y);</span >
It is really OK to silently do the wrong thing if ucol_open fails?
<span class="quote">> Source/JavaScriptCore/runtime/IntlCollatorPrototype.cpp:100
> + const String& sensitivity = collator->sensitivity();</span >
Is this attribute string itself supposed to be case folding or case sensitive? This is not a good pattern, doing string comparisons to convert the collator’s sensitivity setting into arguments for the ICU collator every time we compare a string.
<span class="quote">> Source/JavaScriptCore/runtime/IntlCollatorPrototype.cpp:129
> + return codePointCompare(x, y);</span >
It is really OK to silently do the wrong thing if one of the ucol_setAttribute calls fail?
<span class="quote">> Source/JavaScriptCore/runtime/IntlCollatorPrototype.cpp:134
> + String x16Bit = x.is8Bit() ? String::make16BitFrom8BitSource(x.characters8(), x.length()) : x;
> + String y16Bit = y.is8Bit() ? String::make16BitFrom8BitSource(y.characters8(), y.length()) : y;
> + UCollationResult result = ucol_strcoll(icuCollator, x16Bit.characters16(), x16Bit.length(), y16Bit.characters16(), y16Bit.length());</span >
This is the wrong idiom for making a temporary 16-bit copy of a string. The correct idiom is to use upconvertedCharacters(). It works like this:
UCollationResult result = ucol_strcoll(icuCollator, StringView(x).upconvertedCharacters(), x.length(), StringView(y).upconvertedCharacters(), y.length());
<span class="quote">> Source/JavaScriptCore/runtime/IntlCollatorPrototype.cpp:147
> + switch (result) {
> + case UCOL_EQUAL:
> + return 0;
> + case UCOL_GREATER:
> + return 1;
> + case UCOL_LESS:
> + return -1;
> + }
> + ASSERT_NOT_REACHED();
> + return 0;</span >
It’s OK to try to use some abstraction here and not assume the values, but it’s really not useful. This named constants are just ICU names for these same values. There is no benefit to the switch. This can and should just be:
return result;
<span class="quote">> Source/JavaScriptCore/runtime/IntlCollatorPrototype.cpp:157
> + RELEASE_ASSERT(collator);</span >
I don’t see why we would have this assertion in release code.
<span class="quote">> Source/JavaScriptCore/runtime/IntlObject.cpp:118
> + return String(uloc_getDefault()).replace('_', '-');</span >
We should stop copying and pasting the replace we are doing here.</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>