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

[bugzilla-daemon at webkit.org]

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

Darin Adler <darin at apple.com> changed:

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

--- Comment #31 from Darin Adler <darin at apple.com> ---
Comment on attachment 270575
  --> https://bugs.webkit.org/attachment.cgi?id=270575
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.

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

> Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:103
> +    ASSERT(strlen(currency) == 3 && isAllSpecialCharacters<isASCIIAlpha>(currency, 3));
> +    return (currency[0] << 16) + (currency[1] << 8) + currency[2];

Ditto.

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

Another option would be to use a single function that takes a StringView instead of two overloads.

> Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:194
> +    m_locale = result.get(ASCIILiteral("locale"));

Since we are going to make an Identifier here, not a String, ASCIILiteral is not idea. Might ask Ben Poulain or some other JavaScriptCore experts what the preferred style is for this kind of thing.

> Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:197
> +    m_numberingSystem = result.get(ASCIILiteral("nu"));

Ditto.

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

-- 
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/20160203/4ca19347/attachment.html>