[Webkit-unassigned] [Bug 147602] [INTL] Implement Intl.NumberFormat.prototype.resolvedOptions ()

[bugzilla-daemon at webkit.org]

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

--- Comment #35 from Sukolsak Sakshuwong <sukolsak at gmail.com> ---
(In reply to comment #31)
> Comment on attachment 270575 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=270575&action=review
> 
> > Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:3
> > + * Copyright (C) 2015 Sukolsak Sakshuwong (sukolsak at gmail.com)
> 
> This year is 2016, not 2015.

Fixed.

> > Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:96
> > +    ASSERT(currency.length() == 3 && currency.isAllSpecialCharacters<isASCIIAlpha>());
> 
> This assertion needs to be isASCIIUpper, not just isASCIIAlpha.
> 
> Normally we don’t use && in assertions. Instead we write multiple
> assertions. That way if one clause fires, we know which one it was. In this
> case I think 4 separate assertions would be ideal so we know which one
> failed. 2 would be OK, though.

Done.

> Could also just make computeCurrencySortKey a function template instead of
> writing it twice, except for the length check in the assertion, but there
> are tricks for that too. For example, StringView(currency).length() should
> work with both const String& and const char*.

We have to do ASSERT(isAllSpecialCharacters<isASCIIUpper>(currency, 3)) too. Is there a way to write it so that it works with both const String& and const char*? 

> > Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:240
> > +        currency = currency.upper();
> 
> This should be convertToASCIIUppercase, not upper, for better efficiency,
> and because I am working hard to eliminate upper entirely except for
> possibly one or two call sites.

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/20160210/1bbeaf2f/attachment.html>