CurrencyDigits code in Intl.NumberFormat implementation could be cleaned up

ASSIGNED
Assigned to

Status

()

P5
normal
ASSIGNED
2 years ago
8 months ago

People

(Reporter: littledan, Assigned: littledan)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

2 years ago
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

2 years ago
Component: Untriaged → JavaScript: Internationalization API
Comment hidden (mozreview-request)

Comment 3

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

Comment 5

2 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?
(Assignee)

Updated

2 years ago
Attachment #8841920 - Flags: review?(jwalden+bmo)

Comment 6

2 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+
(Assignee)

Comment 7

2 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

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

Comment 9

2 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.
<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
You need to log in before you can comment on or make changes to this bug.