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)
Core
JavaScript: Internationalization API
Tracking
()
NEW
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.
Comment hidden (mozreview-request) |
Updated•8 years ago
|
Component: Untriaged → JavaScript: Internationalization API
Comment hidden (mozreview-request) |
Comment 3•8 years ago
|
||
mozreview-review |
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 hidden (mozreview-request) |
Reporter | ||
Comment 5•8 years ago
|
||
mozreview-review-reply |
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?
Reporter | ||
Updated•8 years ago
|
Attachment #8841920 -
Flags: review?(jwalden+bmo)
Comment 6•8 years ago
|
||
mozreview-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
::: 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+
Reporter | ||
Comment 7•8 years ago
|
||
mozreview-review-reply |
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>()?
Comment 8•8 years ago
|
||
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
Reporter | ||
Comment 9•8 years ago
|
||
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.
Comment 10•8 years ago
|
||
<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!
Updated•7 years ago
|
Priority: -- → P5
Comment 11•3 years ago
|
||
The bug assignee is inactive on Bugzilla, so the assignee is being reset.
Assignee: littledan → nobody
Status: ASSIGNED → NEW
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•