Bug 147602

Created attachment 264436 [details] Patch

Created attachment 264445 [details] Patch

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

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

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

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

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

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

Created attachment 264455 [details] Patch

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.

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?

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

Created attachment 267620 [details] Patch

Created attachment 267622 [details] Patch

Created attachment 267623 [details] Patch

Created attachment 267624 [details] Patch Fix a linking issue

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.

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?

Created attachment 267730 [details] Patch

(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)

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.

Created attachment 267982 [details] Patch

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

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.

Created attachment 270531 [details] Patch

Created attachment 270532 [details] Patch

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.

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.

Created attachment 270575 [details] Patch

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

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

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

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 } };

Created attachment 271026 [details] Patch

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

Comment on attachment 271026 [details] Patch Clearing flags on attachment: 271026 Committed r196434: <http://trac.webkit.org/changeset/196434>

All reviewed patches have been landed. Closing bug.