[Webkit-unassigned] [Bug 158188] [INTL] Implement the caseFirst option for Intl.Collator

[bugzilla-daemon at webkit.org]

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

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

(In reply to comment #5)
> Comment on attachment 280100 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=280100&action=review
> 
> > Source/JavaScriptCore/runtime/IntlCollator.cpp:289
> >      m_numeric = (result.get(ASCIILiteral("kn")) == "true");
> 
> I’d like how this reads better without the parentheses.

Will fix.

> > Source/JavaScriptCore/runtime/IntlCollator.cpp:290
> > +    const String& caseFirst = result.get(ASCIILiteral("kf"));
> 
> As with the cases below, I think it’s nicer to have the translation from
> string to CaseFirst be in a helper function. It can be inlined if we like.

Will do. Do you have any name suggestion for the helper function? caseFirstString(CaseFirst) <-> caseFirstFromString(const String&)?

> > Source/JavaScriptCore/runtime/IntlCollator.cpp:296
> > +        ASSERT(caseFirst == "false");
> 
> Why can we assert this? What prevents invalid values?

The HashMap "result" which we get the caseFirst value from is sanitized by resolveLocale(...). We test invalid values in the tests.

> > Source/JavaScriptCore/runtime/IntlCollator.cpp:451
> > +const char* IntlCollator::caseFirstString(CaseFirst caseFirst)
> 
> I think we should probably mark this function inline.

Will do.

> > Source/JavaScriptCore/runtime/IntlCollator.cpp:462
> > +    return nullptr;
> 
> I think we should return "false" here instead of nullptr.

Why is that? Is it so that the compiler can optimize out the "case CaseFirst::False:" branch?

-- 
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/20160531/826258d5/attachment.html>