Bug 147602 – [INTL] Implement Intl.NumberFormat.prototype.resolvedOptions ()

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Andy VanWagoner]

Implement ECMA-402 11.3.5 Intl.NumberFormat.prototype.resolvedOptions ()
http://ecma-international.org/publications/standards/Ecma-402.htm

This function provides access to the locale and formatting options computed during initialization of the object.

The function returns a new object whose properties and attributes are set as if constructed by an object literal assigning to each of the following properties the value of the corresponding internal slot of this NumberFormat object (see 11.4): locale, numberingSystem, style, currency, currencyDisplay, minimumIntegerDigits, minimumFractionDigits, maximumFractionDigits, minimumSignificantDigits, maximumSignificantDigits, and useGrouping. Properties whose corresponding internal slots are not present are not assigned.

[Sukolsak Sakshuwong]

Created attachment 264436 [details]
Patch

[Sukolsak Sakshuwong]

Created attachment 264445 [details]
Patch

[Build Bot]

Comment on attachment 264445 [details]
Patch

Attachment 264445 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/360618

New failing tests:
js/intl-numberformat.html

[Build Bot]

Created attachment 264450 [details]
Archive of layout-test-results from ews114 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 264445 [details]
Patch

Attachment 264445 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/360658

New failing tests:
js/intl-numberformat.html

[Build Bot]

Created attachment 264451 [details]
Archive of layout-test-results from ews100 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 264445 [details]
Patch

Attachment 264445 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/360665

New failing tests:
js/intl-numberformat.html

[Build Bot]

Created attachment 264452 [details]
Archive of layout-test-results from ews105 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Sukolsak Sakshuwong]

Created attachment 264455 [details]
Patch

[Geoffrey Garen]

Comment on attachment 264455 [details]
Patch

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

r=me

> Source/JavaScriptCore/runtime/IntlNumberFormat.h:87
> +    unsigned m_minimumIntegerDigits;
> +    unsigned m_minimumFractionDigits;
> +    unsigned m_maximumFractionDigits;

These need initializers.

> Source/JavaScriptCore/runtime/IntlNumberFormat.h:91
> +    bool m_useGrouping;

And this.

> Source/JavaScriptCore/runtime/IntlNumberFormatConstructor.cpp:71
> +#ifndef NDEBUG
> +    ASSERT(key == "nu");
> +#else
> +    UNUSED_PARAM(key);
> +#endif

You can use ASSERT_UNUSED here instead.

> Source/JavaScriptCore/runtime/IntlNumberFormatConstructor.cpp:73
> +    Vector<String> keyLocaleData({

Can we use std::array here instead? We don't intend to grow or shrink this data structure.

> Source/JavaScriptCore/runtime/IntlNumberFormatPrototype.cpp:156
> +    options->putDirect(vm, Identifier::fromString(&vm, "minimumIntegerDigits"), jsNumber(nf->minimumIntegerDigits()));
> +    options->putDirect(vm, Identifier::fromString(&vm, "minimumFractionDigits"), jsNumber(nf->minimumFractionDigits()));
> +    options->putDirect(vm, Identifier::fromString(&vm, "maximumFractionDigits"), jsNumber(nf->maximumFractionDigits()));
> +    if (nf->minimumSignificantDigits())
> +        options->putDirect(vm, Identifier::fromString(&vm, "minimumSignificantDigits"), jsNumber(nf->minimumSignificantDigits()));
> +    if (nf->maximumSignificantDigits())
> +        options->putDirect(vm, Identifier::fromString(&vm, "maximumSignificantDigits"), jsNumber(nf->maximumSignificantDigits()));
> +    options->putDirect(vm, Identifier::fromString(&vm, "useGrouping"), jsBoolean(nf->useGrouping()));

I think we want these to be CommonIdentifiers so we don't have to keep allocating and hashing them.

[Sukolsak Sakshuwong]

Thanks for the review

(In reply to comment #10)
> > Source/JavaScriptCore/runtime/IntlNumberFormat.h:87
> > +    unsigned m_minimumIntegerDigits;
> > +    unsigned m_minimumFractionDigits;
> > +    unsigned m_maximumFractionDigits;
> 
> These need initializers.
> 
> > Source/JavaScriptCore/runtime/IntlNumberFormat.h:91
> > +    bool m_useGrouping;
> 
> And this.

The only functions that read these attributes are Intl.NumberFormat.prototype.resolvedOptions and (in the upcoming patch) Intl.NumberFormat.prototype.format. They always check that the NumberFormat object has been initialized. If not, they initialize it. These attributes are always set in the initialization. So, I think it's fine to not have initializers for these. What do you think?

Related question: The only time resolvedOptions() is called, but the NumberFormat object has not been initialized, is when we call "Intl.NumberFormat.prototype.resolvedOptions()". The spec says that "The Intl.NumberFormat prototype object is itself an Intl.NumberFormat instance ... whose internal slots are set as if it had been constructed by the expression Construct(%NumberFormat%)." But I don't want to populate the internal slots of the prototype eagerly, because Intl.NumberFormat.prototype is created when JSGlobalObject is created. Is it fine to do lazy initialization like this?

> > Source/JavaScriptCore/runtime/IntlNumberFormatConstructor.cpp:73
> > +    Vector<String> keyLocaleData({
> 
> Can we use std::array here instead? We don't intend to grow or shrink this
> data structure.

The localeData() function is passed to the resolveLocale() function, which expects a function pointer of type Vector<String> (*localeData)(const String&, const String&). I can't change the return type to std::array because vectors of different size are also passed to it.

> > Source/JavaScriptCore/runtime/IntlNumberFormatPrototype.cpp:156
> > +    options->putDirect(vm, Identifier::fromString(&vm, "minimumIntegerDigits"), jsNumber(nf->minimumIntegerDigits()));
> > +    options->putDirect(vm, Identifier::fromString(&vm, "minimumFractionDigits"), jsNumber(nf->minimumFractionDigits()));
> > +    options->putDirect(vm, Identifier::fromString(&vm, "maximumFractionDigits"), jsNumber(nf->maximumFractionDigits()));
> > +    if (nf->minimumSignificantDigits())
> > +        options->putDirect(vm, Identifier::fromString(&vm, "minimumSignificantDigits"), jsNumber(nf->minimumSignificantDigits()));
> > +    if (nf->maximumSignificantDigits())
> > +        options->putDirect(vm, Identifier::fromString(&vm, "maximumSignificantDigits"), jsNumber(nf->maximumSignificantDigits()));
> > +    options->putDirect(vm, Identifier::fromString(&vm, "useGrouping"), jsBoolean(nf->useGrouping()));
> 
> I think we want these to be CommonIdentifiers so we don't have to keep
> allocating and hashing them.

There are only two places that use these identifiers: the constructor and prototype.resolvedOptions(), which probably won't be called very often. Should I still add these to CommonIdentifiers?

[Geoffrey Garen]

> > These need initializers.
> > 
> > > Source/JavaScriptCore/runtime/IntlNumberFormat.h:91
> > > +    bool m_useGrouping;
> > 
> > And this.
> 
> The only functions that read these attributes are
> Intl.NumberFormat.prototype.resolvedOptions and (in the upcoming patch)
> Intl.NumberFormat.prototype.format. They always check that the NumberFormat
> object has been initialized. If not, they initialize it. These attributes
> are always set in the initialization. So, I think it's fine to not have
> initializers for these. What do you think?

I think you're right that the code will be correct without initializers.

Still, it is good idiomatic practice to initialize from the start so that readers of this code do not need to think about issues like this.

The only case where I would leave out an initializer is a case where performance is super-critical and removing the initial write saves a large fraction of time. I don't think this is one of those cases.

> Related question: The only time resolvedOptions() is called, but the
> NumberFormat object has not been initialized, is when we call
> "Intl.NumberFormat.prototype.resolvedOptions()". The spec says that "The
> Intl.NumberFormat prototype object is itself an Intl.NumberFormat instance
> ... whose internal slots are set as if it had been constructed by the
> expression Construct(%NumberFormat%)." But I don't want to populate the
> internal slots of the prototype eagerly, because Intl.NumberFormat.prototype
> is created when JSGlobalObject is created. Is it fine to do lazy
> initialization like this?

I think lazy initialization is OK. I'm not sure exactly how I would engineer it in this case.

> 
> > > Source/JavaScriptCore/runtime/IntlNumberFormatConstructor.cpp:73
> > > +    Vector<String> keyLocaleData({
> > 
> > Can we use std::array here instead? We don't intend to grow or shrink this
> > data structure.
> 
> The localeData() function is passed to the resolveLocale() function, which
> expects a function pointer of type Vector<String> (*localeData)(const
> String&, const String&). I can't change the return type to std::array
> because vectors of different size are also passed to it.

Okeedokee.

> 
> > > Source/JavaScriptCore/runtime/IntlNumberFormatPrototype.cpp:156
> > > +    options->putDirect(vm, Identifier::fromString(&vm, "minimumIntegerDigits"), jsNumber(nf->minimumIntegerDigits()));
> > > +    options->putDirect(vm, Identifier::fromString(&vm, "minimumFractionDigits"), jsNumber(nf->minimumFractionDigits()));
> > > +    options->putDirect(vm, Identifier::fromString(&vm, "maximumFractionDigits"), jsNumber(nf->maximumFractionDigits()));
> > > +    if (nf->minimumSignificantDigits())
> > > +        options->putDirect(vm, Identifier::fromString(&vm, "minimumSignificantDigits"), jsNumber(nf->minimumSignificantDigits()));
> > > +    if (nf->maximumSignificantDigits())
> > > +        options->putDirect(vm, Identifier::fromString(&vm, "maximumSignificantDigits"), jsNumber(nf->maximumSignificantDigits()));
> > > +    options->putDirect(vm, Identifier::fromString(&vm, "useGrouping"), jsBoolean(nf->useGrouping()));
> > 
> > I think we want these to be CommonIdentifiers so we don't have to keep
> > allocating and hashing them.
> 
> There are only two places that use these identifiers: the constructor and
> prototype.resolvedOptions(), which probably won't be called very often.
> Should I still add these to CommonIdentifiers?

I guess it's a close call. Let's leave it as strings for now.

[Sukolsak Sakshuwong]

Created attachment 267620 [details]
Patch

[Sukolsak Sakshuwong]

Created attachment 267622 [details]
Patch

[Sukolsak Sakshuwong]

Created attachment 267623 [details]
Patch

[Sukolsak Sakshuwong]

Created attachment 267624 [details]
Patch

Fix a linking issue

[Sukolsak Sakshuwong]

I've made several changes to this patch to bring it into line with the Intl.Collator patch (Bug 147604).

(In reply to comment #10)
> Comment on attachment 264455 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=264455&action=review
> 
> r=me
> 
> > Source/JavaScriptCore/runtime/IntlNumberFormat.h:87
> > +    unsigned m_minimumIntegerDigits;
> > +    unsigned m_minimumFractionDigits;
> > +    unsigned m_maximumFractionDigits;
> 
> These need initializers.
> 
> > Source/JavaScriptCore/runtime/IntlNumberFormat.h:91
> > +    bool m_useGrouping;
> 
> And this.

Done.

> > Source/JavaScriptCore/runtime/IntlNumberFormatConstructor.cpp:71
> > +#ifndef NDEBUG
> > +    ASSERT(key == "nu");
> > +#else
> > +    UNUSED_PARAM(key);
> > +#endif
> 
> You can use ASSERT_UNUSED here instead.

Done.

[Darin Adler]

Comment on attachment 267624 [details]
Patch

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

> Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:92
> +    Vector<String> keyLocaleData({

Seems unfortunate to re-create this vector every time the function is called. Might consider changing the function’s interface to return a const& unless it can involve computing a unique value and using a static NeverDestroyed for this particular case.

> Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:158
> +    for (const auto& currencyMinorUnit : currencyMinorUnits) {

This list might be long enough that's binary search would be faster than a linear one.

> Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:159
> +        if (currency == currencyMinorUnit.first)

Is it correct that this is a case sensitive check?

> Source/JavaScriptCore/runtime/IntlObject.cpp:239
> +        if (std::isnan(doubleValue) || doubleValue < static_cast<double>(minimum) || doubleValue > static_cast<double>(maximum)) {

There is a way to write this without an explicit check for NaN.

    If (!(x >= min && x <= max))

> Source/JavaScriptCore/runtime/IntlObject.cpp:245
> +        return static_cast<unsigned>(doubleValue);

This has peculiar behavior for max + 0.1; do we have test cases that cover values just slightly under min and slightly over max?

[Sukolsak Sakshuwong]

Created attachment 267730 [details]
Patch

[Sukolsak Sakshuwong]

(In reply to comment #18)
> Comment on attachment 267624 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=267624&action=review
> 
> > Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:92
> > +    Vector<String> keyLocaleData({
> 
> Seems unfortunate to re-create this vector every time the function is
> called. Might consider changing the function’s interface to return a const&
> unless it can involve computing a unique value and using a static
> NeverDestroyed for this particular case.

This function is passed to resolveLocale. Other classes that use resolveLocale can pass functions that compute a unique value to it, such as sortLocaleData in IntlCollator.cpp.

There is a way to optimize it, which requires some changes to the algorithm in the spec. But I think we should wait until all Intl classes are implemented first.

> > Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:158
> > +    for (const auto& currencyMinorUnit : currencyMinorUnits) {
> 
> This list might be long enough that's binary search would be faster than a
> linear one.

Done. Not sure if I do it correctly. Could you please take a look?

> > Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:159
> > +        if (currency == currencyMinorUnit.first)
> 
> Is it correct that this is a case sensitive check?

Yes, it is correct. Before we call we this function, we convert the currency to uppercase letters.

> > Source/JavaScriptCore/runtime/IntlObject.cpp:239
> > +        if (std::isnan(doubleValue) || doubleValue < static_cast<double>(minimum) || doubleValue > static_cast<double>(maximum)) {
> 
> There is a way to write this without an explicit check for NaN.
> 
>     If (!(x >= min && x <= max))

Done. Nice trick!

> > Source/JavaScriptCore/runtime/IntlObject.cpp:245
> > +        return static_cast<unsigned>(doubleValue);
> 
> This has peculiar behavior for max + 0.1; do we have test cases that cover
> values just slightly under min and slightly over max?

Yes, we have test cases for those cases. For example, the allowed range of minimumIntegerDigits is [1, 21]. So, we test
- 0    (throws)
- 0.9  (throws)
- 1
- 21
- 21.1 (throws)
- 22   (throws)

[Andy VanWagoner]

Comment on attachment 267730 [details]
Patch

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

> Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:91
> +    // FIXME: Use ICU to find the default numbering system for the given locale.

getNumberingSystemsForLocale has been added in IntlObject. It uses the enumeration from ICU, and also will give the correct default for a locale.

[Sukolsak Sakshuwong]

Created attachment 267982 [details]
Patch

[Sukolsak Sakshuwong]

(In reply to comment #21)
> > Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:91
> > +    // FIXME: Use ICU to find the default numbering system for the given locale.
> 
> getNumberingSystemsForLocale has been added in IntlObject. It uses the
> enumeration from ICU, and also will give the correct default for a locale.

Thanks. Fixed.

[Darin Adler]

Comment on attachment 267982 [details]
Patch

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

Seems like a reasonable first cut. We need to remember to do some performance testing and tuning on this; there’s a lot of memory allocation here.

> Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:94
> +static bool isWellFormedCurrencyCode(const String& currency)

Might be worth marking some of these super-simple functions inline, especially if they have only one or two call sites.

> Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:100
> +static unsigned computeCurrencySortKey(const char* currency)

Ditto.

> Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:106
> +static unsigned extractCurrencySortKey(std::pair<const char*, unsigned>* currencyMinorUnit)

Ditto.

> Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:143
> +    std::array<std::pair<const char*, unsigned>, 26> currencyMinorUnits = { {
> +        { "BHD", 3 },
> +        { "BIF", 0 },
> +        { "BYR", 0 },
> +        { "CLF", 4 },
> +        { "CLP", 0 },
> +        { "DJF", 0 },
> +        { "GNF", 0 },
> +        { "IQD", 3 },
> +        { "ISK", 0 },
> +        { "JOD", 3 },
> +        { "JPY", 0 },
> +        { "KMF", 0 },
> +        { "KRW", 0 },
> +        { "KWD", 3 },
> +        { "LYD", 3 },
> +        { "OMR", 3 },
> +        { "PYG", 0 },
> +        { "RWF", 0 },
> +        { "TND", 3 },
> +        { "UGX", 0 },
> +        { "UYI", 0 },
> +        { "VND", 0 },
> +        { "VUV", 0 },
> +        { "XAF", 0 },
> +        { "XOF", 0 },
> +        { "XPF", 0 }
> +    } };

I believe this will do some work every time the function is called. We need to make sure this actually gets compiled in a way that doesn’t do the array initialization each time, for efficiently. Might need to eschew the use of std::array, or mark this as const, to achieve that.

> Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:144
> +    std::pair<const char*, unsigned>* currencyMinorUnit = tryBinarySearch<std::pair<const char*, unsigned>>(currencyMinorUnits, currencyMinorUnits.size(), computeCurrencySortKey(currency.utf8().data()), extractCurrencySortKey);

Would read better with the use of auto*. Unclear on why we need to explicitly pass the type in when invoking the tryBinarySearch function template; I’d expect it to work without specifying that explicitly.

Should overload computeCurrencySortKey to work on a const String& or StringView correctly; doing memory allocation just to convert the String to a CString, just to get the first three characters of the string, is not great.

[Sukolsak Sakshuwong]

Created attachment 270531 [details]
Patch

[Sukolsak Sakshuwong]

Created attachment 270532 [details]
Patch

[Sukolsak Sakshuwong]

Thanks.

(In reply to comment #24)
> Comment on attachment 267982 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=267982&action=review
> 
> Seems like a reasonable first cut. We need to remember to do some
> performance testing and tuning on this; there’s a lot of memory allocation
> here.
> 
> > Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:94
> > +static bool isWellFormedCurrencyCode(const String& currency)
> 
> Might be worth marking some of these super-simple functions inline,
> especially if they have only one or two call sites.

Inlined.

> > Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:100
> > +static unsigned computeCurrencySortKey(const char* currency)
> 
> Ditto.

Marked inline.

> > Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:106
> > +static unsigned extractCurrencySortKey(std::pair<const char*, unsigned>* currencyMinorUnit)
> 
> Ditto.

I am not sure if it will make a difference, because we don't call this function here. We pass it to tryBinarySearch.

> > Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:143
> > +    std::array<std::pair<const char*, unsigned>, 26> currencyMinorUnits = { {
> > +        { "BHD", 3 },
> > +        { "BIF", 0 },
> > +        { "BYR", 0 },
> > +        { "CLF", 4 },
> > +        { "CLP", 0 },
> > +        { "DJF", 0 },
> > +        { "GNF", 0 },
> > +        { "IQD", 3 },
> > +        { "ISK", 0 },
> > +        { "JOD", 3 },
> > +        { "JPY", 0 },
> > +        { "KMF", 0 },
> > +        { "KRW", 0 },
> > +        { "KWD", 3 },
> > +        { "LYD", 3 },
> > +        { "OMR", 3 },
> > +        { "PYG", 0 },
> > +        { "RWF", 0 },
> > +        { "TND", 3 },
> > +        { "UGX", 0 },
> > +        { "UYI", 0 },
> > +        { "VND", 0 },
> > +        { "VUV", 0 },
> > +        { "XAF", 0 },
> > +        { "XOF", 0 },
> > +        { "XPF", 0 }
> > +    } };
> 
> I believe this will do some work every time the function is called. We need
> to make sure this actually gets compiled in a way that doesn’t do the array
> initialization each time, for efficiently. Might need to eschew the use of
> std::array, or mark this as const, to achieve that.

Changed it to std::pair<const char*, unsigned>[]. I tried using std::initializer_list but got an error saying that it "does not provide a subscript operator".

> > Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:144
> > +    std::pair<const char*, unsigned>* currencyMinorUnit = tryBinarySearch<std::pair<const char*, unsigned>>(currencyMinorUnits, currencyMinorUnits.size(), computeCurrencySortKey(currency.utf8().data()), extractCurrencySortKey);
> 
> Would read better with the use of auto*. Unclear on why we need to
> explicitly pass the type in when invoking the tryBinarySearch function
> template; I’d expect it to work without specifying that explicitly.

Used auto*. It looks like we have to explicitly pass the type. Otherwise, I get this error:

/WebKit/Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:145:31: error: no matching function for call to 'tryBinarySearch'
    auto* currencyMinorUnit = tryBinarySearch(currencyMinorUnits, WTF_ARRAY_LENGTH(currencyMinorUnits), computeCurrencySortKey(StringView(currency)), extractCurrencySortKey);
                              ^~~~~~~~~~~~~~~

...

/WebKit/WebKitBuild/Debug/usr/local/include/wtf/StdLibExtras.h:240:26: note: candidate template ignored: couldn't infer template argument 'ArrayElementType'
inline ArrayElementType* tryBinarySearch(ArrayType& array, size_t size, KeyType key, ExtractKey extractKey = ExtractKey())

> Should overload computeCurrencySortKey to work on a const String& or
> StringView correctly; doing memory allocation just to convert the String to
> a CString, just to get the first three characters of the string, is not
> great.

Done.

[Darin Adler]

Comment on attachment 270532 [details]
Patch

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

> Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:103
> +    return (currency[0] << 16) + (currency[1] << 8) + currency[2];

This will do the wrong thing if any characters are in the range 0x80-0xFF on platforms where char is signed. Should we assert the three characters are all ASCII?

> Source/JavaScriptCore/runtime/IntlObject.cpp:241
> +        if (!(doubleValue >= static_cast<double>(minimum) && doubleValue <= static_cast<double>(maximum))) {

Are these static_cast necessary? Doesn’t the unsigned automatically get promoted to a double without explicit conversion?

> Source/JavaScriptCore/runtime/IntlObject.cpp:242
> +            state.vm().throwException(&state, createRangeError(&state, property.publicName() + ASCIILiteral(" is out of range")));

I don’t think you need the ASCIILiteral here, and in fact I think that using it makes the code slightly less efficient, causing it to allocate an additional temporary string.

[Sukolsak Sakshuwong]

Created attachment 270575 [details]
Patch

[Sukolsak Sakshuwong]

(In reply to comment #28)
> Comment on attachment 270532 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=270532&action=review
> 
> > Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:103
> > +    return (currency[0] << 16) + (currency[1] << 8) + currency[2];
> 
> This will do the wrong thing if any characters are in the range 0x80-0xFF on
> platforms where char is signed. Should we assert the three characters are
> all ASCII?

Done. Use isASCIIAlpha (as opposed to isASCII) to be consistent with the spec.

> > Source/JavaScriptCore/runtime/IntlObject.cpp:241
> > +        if (!(doubleValue >= static_cast<double>(minimum) && doubleValue <= static_cast<double>(maximum))) {
> 
> Are these static_cast necessary? Doesn’t the unsigned automatically get
> promoted to a double without explicit conversion?

Removed static_cast.

> > Source/JavaScriptCore/runtime/IntlObject.cpp:242
> > +            state.vm().throwException(&state, createRangeError(&state, property.publicName() + ASCIILiteral(" is out of range")));
> 
> I don’t think you need the ASCIILiteral here, and in fact I think that using
> it makes the code slightly less efficient, causing it to allocate an
> additional temporary string.

Done. Need to add "*" in front of property.publicName().

[Darin Adler]

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 ([e-mail])

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.

[Sukolsak Sakshuwong]

(In reply to comment #31)
> > 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.

Could you please clarify? "result" is a HashMap<String, String>. So, I think this is fine.

[Sukolsak Sakshuwong]

For another optimization, we could use constexpr on computeCurrencySortKey and pre-compute the sort keys like the following. But then, we couldn't use ASSERT inside computeCurrencySortKey. What are your thoughts on this?

static constexpr unsigned computeCurrencySortKey(const char* currency)
{
    return (currency[0] << 16) + (currency[1] << 8) + currency[2];
}

std::pair<unsigned, unsigned> currencyMinorUnits[] = {
    { computeCurrencySortKey("BHD"), 3 },
    { computeCurrencySortKey("BIF"), 0 },
    { computeCurrencySortKey("BYR"), 0 },
    ...
    { computeCurrencySortKey("XPF"), 0 }
};

[Sukolsak Sakshuwong]

Created attachment 271026 [details]
Patch

[Sukolsak Sakshuwong]

(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 ([e-mail])
> 
> 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.

[WebKit Commit Bot]

Comment on attachment 271026 [details]
Patch

Clearing flags on attachment: 271026

Committed r196434: <http://trac.webkit.org/changeset/196434>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.