[Webkit-unassigned] [Bug 147604] [INTL] Implement Collator Compare Functions

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Dec 16 08:57:48 PST 2015


https://bugs.webkit.org/show_bug.cgi?id=147604

Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #267384|review?                     |review+
              Flags|                            |

--- Comment #23 from Darin Adler <darin at apple.com> ---
Comment on attachment 267384
  --> https://bugs.webkit.org/attachment.cgi?id=267384
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=267384&action=review

> Source/JavaScriptCore/runtime/IntlCollator.cpp:277
> +    m_numeric = (result.get(ASCIILiteral("kn")) == "true");

Doesn’t seem to be checking the type for boolean like the pseudocode says we should. Do we have tests covering that?

> Source/JavaScriptCore/runtime/IntlCollator.cpp:336
> +    UColAttributeValue strength;

I suggest initializing this to UCOL_PRIMARY. Would fix a warning on Windows.

> Source/JavaScriptCore/runtime/IntlCollator.cpp:396
> +    UCharIterator iteratorX = createIterator(x);
> +    UCharIterator iteratorY = createIterator(y);

Looks like there is a linking problem on Windows.

     Creating library C:/cygwin/home/buildbot/WebKit/WebKitBuild/Release/lib32/JavaScriptCore.lib and object C:/cygwin/home/buildbot/WebKit/WebKitBuild/Release/lib32/JavaScriptCore.exp
IntlCollator.obj : error LNK2019: unresolved external symbol "struct UCharIterator __cdecl WTF::createIterator(class WTF::StringView)" (?createIterator at WTF@@YA?AUUCharIterator@@VStringView at 1@@Z) referenced in function "public: class JSC::JSValue __thiscall JSC::IntlCollator::compareStrings(class JSC::ExecState &,class WTF::StringView,class WTF::StringView)" (?compareStrings at IntlCollator@JSC@@QAE?AVJSValue at 2@AAVExecState at 2@VStringView at WTF@@1 at Z) [C:\cygwin\home\buildbot\WebKit\WebKitBuild\Release\Source\JavaScriptCore\JavaScriptCore.vcxproj]

I think this is because the declaration of createIterator in Collator.h needs WTF_EXPORT_PRIVATE.

> Source/JavaScriptCore/runtime/IntlCollator.cpp:397
> +    int result = ucol_strcollIter(m_collator, &iteratorX, &iteratorY, &status);

auto is slightly better than int here

> Source/JavaScriptCore/runtime/IntlCollatorConstructor.cpp:111
> +    if (state->hadException())
> +        return JSValue::encode(jsUndefined());

We don’t need this code. There is no harm in returning the collator when there’s an exception; the return value will be ignored. It would be better to omit this.

> Source/JavaScriptCore/runtime/IntlCollatorConstructor.cpp:133
> +    if (state->hadException())
> +        return JSValue::encode(jsUndefined());

We don’t need this code. There is no harm in returning the collator when there’s an exception; the return value will be ignored. It would be better to omit this.

> Source/JavaScriptCore/runtime/IntlObject.cpp:118
> +    String locale(uloc_getDefault());

We normally prefer the = syntax to the function call style syntax for code like this.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20151216/6a1ab164/attachment.html>


More information about the webkit-unassigned mailing list