Open Bug 1342647 Opened 8 years ago Updated 10 months ago

CurrencyDigits code in Intl.NumberFormat implementation could be cleaned up

Categories

(Core :: JavaScript: Internationalization API, defect, P5)

defect

Tracking

()

People

(Reporter: littledan, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/55.0.2883.87 Safari/537.36 Steps to reproduce: In Intl.NumberFormat, I was wondering how SpiderMonkey gets the default number of digits to display for a currency. Actual results: It turns out CurrencyDigits is periodically scraped from somewhere into a JavaScript file. Expected results: We should be calling into ucurr_getDefaultFractionDigits , an ICU API which solves exactly this problem. I have a patch in progress to do this.
Component: Untriaged → JavaScript: Internationalization API
Comment on attachment 8841169 [details] Bug 1342647 - Remove redundant IntlCurrency system and use ICU, https://reviewboard.mozilla.org/r/115488/#review117224 ::: js/src/builtin/Intl.cpp:1385 (Diff revision 2) > + if (!stableChars.initTwoByte(cx, str)) > + return false; > + mozilla::Range<const char16_t> chars = stableChars.twoByteRange(); > + // This might not be null-terminated, but ICU only ever looks > + // at the first three UChars, since an ISO currency code always has a > + // length of three e_e Hmm, this dependence is a bit squirrely. But also, if this is really true, it suggests we're being super-inefficient here. Stable-stringing and other gunk is a ton of overhead for just three chars -- might as well just do it manually into a stacked array. So how about this instead: char16_t chars[4]; { JSLinearString* currency = args[0].toString()->ensureLinear(cx); if (!currency) return false; MOZ_ASSERT(currency->length() == 3); for (size_t i = 0; i < 3; i++) chars[i] = currency->latin1OrTwoByteChar(i); chars[3] = '\0'; } so that we're not relying on this weird not-actually-null-terminated thing.
Attachment #8841169 - Flags: review?(jwalden+bmo) → review-
Comment on attachment 8841169 [details] Bug 1342647 - Remove redundant IntlCurrency system and use ICU, https://reviewboard.mozilla.org/r/115488/#review117224 > e_e > > Hmm, this dependence is a bit squirrely. But also, if this is really true, it suggests we're being super-inefficient here. Stable-stringing and other gunk is a ton of overhead for just three chars -- might as well just do it manually into a stacked array. So how about this instead: > > char16_t chars[4]; > { > JSLinearString* currency = args[0].toString()->ensureLinear(cx); > if (!currency) > return false; > > MOZ_ASSERT(currency->length() == 3); > for (size_t i = 0; i < 3; i++) > chars[i] = currency->latin1OrTwoByteChar(i); > chars[3] = '\0'; > } > > so that we're not relying on this weird not-actually-null-terminated thing. Good point; I was uncomfortable with the phrasing too, just not sure how to do it right in SpiderMonkey. I changed the implementation to roughly what you wrote. What do you think?
Attachment #8841920 - Flags: review?(jwalden+bmo)
Comment on attachment 8841920 [details] Use simpler method of conversion and always get a null-terminated string https://reviewboard.mozilla.org/r/115974/#review117472 ::: js/src/builtin/Intl.cpp:1378 (Diff revision 1) > { > CallArgs args = CallArgsFromVp(argc, vp); > MOZ_ASSERT(args.length() == 1); > MOZ_ASSERT(args[0].isString()); > > - RootedString str(cx, args[0].toString()); > + UChar chars[4]; It's somewhat unusual for us to use UChar other than as the result of a BlahToUChar, but yeah, I guess it makes sense to use it directly here, without the char16_t go-between. Can't wait for new-enough ICU where we can drop the UChar pretense entirely. (Well, I guess maybe Windows holds us back there. Never mind. Pfui.)
Attachment #8841920 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 8841920 [details] Use simpler method of conversion and always get a null-terminated string https://reviewboard.mozilla.org/r/115974/#review117472 > It's somewhat unusual for us to use UChar other than as the result of a BlahToUChar, but yeah, I guess it makes sense to use it directly here, without the char16_t go-between. Can't wait for new-enough ICU where we can drop the UChar pretense entirely. (Well, I guess maybe Windows holds us back there. Never mind. Pfui.) Would you prefer an explicit static_cast<UChar>()?
No need for static_cast<>, if I'd wanted it I would have said so explicitly and probably r-'d the patch. :-) Mozreview is a dumb and doesn't want to let me do pushbutton try-pushing (wonder if my LDAP bits are busted somehow as far as Mozreview is concerned -- I don't use it myself, so it could well have been so broken for years), so I had to export the raw patch and push it manually. https://treeherder.mozilla.org/#/jobs?repo=try&revision=35c1b6b0365d2e06308557d1b22e947bcb4a0c71 That's based on some random inbound rev, so be aware that a failure in the push is not *necessarily* an indication that this patch is bad. A quick skim of the first bit of |hg in| suggests it should be good -- no backouts of stuff that hadn't been landed since my pull -- but that's a fallible heuristic.
Assignee: nobody → littledan
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Sorry, this patch is on hold since it actually introduces a spec violation, which is why it fails the automated tests. IMO the new behavior is actually better than the old, but the standards discussion needs to come first. See https://github.com/tc39/ecma402/issues/134 for more details.
<3 tests. No need to have to explicitly consider the wider world of what might be known to be mistaken if it'll get caught in automation!
Priority: -- → P5

The bug assignee is inactive on Bugzilla, so the assignee is being reset.

Assignee: littledan → nobody
Status: ASSIGNED → NEW
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: