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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Dec 16 11:09:36 PST 2015


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

--- Comment #25 from Sukolsak Sakshuwong <sukolsak at gmail.com> ---
Thanks!

(In reply to comment #23)
> Comment on attachment 267384 [details]
> 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?

The pseudocode says "If the name given in the Type column of the row is 'boolean', let value be the result of comparing value with 'true'." "Type" here refers to the text in the Type column of Table 1 in the spec, which looks like the following:

-------------------------------------------------------
Key | Property  | Type      | Values
-------------------------------------------------------
kn  | numeric   | "boolean" |
kf  | caseFirst | "string"  | "upper", "lower", "false"
-------------------------------------------------------

We have tests covering numeric of various values. See http://trac.webkit.org/browser/trunk/LayoutTests/js/script-tests/intl-collator.js#L88 We test

- {numeric: true}
- {numeric: false}  (Only this one results in m_numeric == false)
- {numeric: 'false'}
- {numeric: { }}
- { get numeric() { throw 42; } }

> > Source/JavaScriptCore/runtime/IntlCollator.cpp:336
> > +    UColAttributeValue strength;
> 
> I suggest initializing this to UCOL_PRIMARY. Would fix a warning on Windows.

Done.

> > Source/JavaScriptCore/runtime/IntlCollator.cpp:396
> > +    UCharIterator iteratorX = createIterator(x);
> > +    UCharIterator iteratorY = createIterator(y);
> 
> Looks like there is a linking problem on Windows.
> 

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

Done.

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

Done.

> > 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.

Done.

> > 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.

Done.

-- 
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/d5b2008a/attachment.html>


More information about the webkit-unassigned mailing list