[INTL] Call Typed Array elements toLocaleString with locale and options
Created attachment 340768 [details] Patch
This behavior is currently observable in SpiderMonkey. I don't believe v8 has yet adopted it. test262/intl402 specifically tests for this behavior. I would imagine eventually TypedArray toLocaleString and Array toLocaleString will use some of the semantics of ListFormat[1], but for now this at least passes NumberFormat locale and options to each number's toLocaleString in a typed array. [1] https://github.com/tc39/proposal-intl-list-format
Is there anything else this patch needs before it can be considered?
Created attachment 343721 [details] Rebased
Comment on attachment 343721 [details] Rebased Rejecting attachment 343721 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'apply-attachment', '--no-update', '--non-interactive', 343721, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Logging in as commit-queue@webkit.org... Fetching: https://bugs.webkit.org/attachment.cgi?id=343721&action=edit Fetching: https://bugs.webkit.org/show_bug.cgi?id=185796&ctype=xml&excludefield=attachmentdata Processing 1 patch from 1 bug. Processing patch 343721 from bug 185796. Fetching: https://bugs.webkit.org/attachment.cgi?id=343721 Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Keith Miller']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit Parsed 4 diffs from patch file(s). patching file Source/JavaScriptCore/ChangeLog Hunk #1 succeeded at 1 with fuzz 3. patching file Source/JavaScriptCore/builtins/TypedArrayPrototype.js patching file JSTests/ChangeLog Hunk #1 succeeded at 1 with fuzz 3. patching file JSTests/test262/expectations.yaml Hunk #1 FAILED at 1825. 1 out of 1 hunk FAILED -- saving rejects to file JSTests/test262/expectations.yaml.rej Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Keith Miller']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit Full output: https://webkit-queues.webkit.org/results/8651222
Created attachment 345765 [details] Patch
Comment on attachment 345765 [details] Patch Clearing flags on attachment: 345765 Committed r234207: <https://trac.webkit.org/changeset/234207>
All reviewed patches have been landed. Closing bug.
<rdar://problem/42589763>
Comment on attachment 345765 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=345765&action=review > Source/JavaScriptCore/builtins/TypedArrayPrototype.js:410 > - var string = this[0].toLocaleString(); > + var string = this[0].toLocaleString(@argument(0), @argument(1)); > for (var i = 1; i < length; i++) > - string += "," + this[i].toLocaleString(); > + string += "," + this[i].toLocaleString(@argument(0), @argument(1)); Presumably the toLocaleString function can do any arbitrary work, so it can examine its arguments any way it likes. This will turn "no arguments" into "undefined, undefined", and will turn three arguments into two; these behaviors might be OK and might not be. I’m not clear what the specification calls for here. Do we want, instead, to forward the entire array of arguments, without changing its length?
Comment on attachment 345765 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=345765&action=review >> Source/JavaScriptCore/builtins/TypedArrayPrototype.js:410 >> + string += "," + this[i].toLocaleString(@argument(0), @argument(1)); > > Presumably the toLocaleString function can do any arbitrary work, so it can examine its arguments any way it likes. This will turn "no arguments" into "undefined, undefined", and will turn three arguments into two; these behaviors might be OK and might not be. I’m not clear what the specification calls for here. Do we want, instead, to forward the entire array of arguments, without changing its length? This appears to be what the specification in Intl 402 asks for: https://tc39.github.io/ecma402/#sup-array.prototype.tolocalestring.
I read the spec in the way the patch has it, but at least for the default Number.prototype.toLocaleString, passing along the arguments unaltered, either by using apply, or with rest args and spread args would have the same effect. For places where the Number.prototype.toLocaleString has been altered, I'd expect it to receive [ undefined, undefined ] from the text of the spec. As a quick test I ran the following in Firefox: ``` Number.prototype.toLocaleString = function () { console.log(arguments) } new Uint8Array([ 5 ]).toLocaleString() ``` which gave me: ``` Arguments { 0: undefined, 1: undefined } ``` That seems to match the implementation in this patch.
(In reply to Keith Miller from comment #11) > This appears to be what the specification in Intl 402 asks for: > > https://tc39.github.io/ecma402/#sup-array.prototype.tolocalestring. OK, great. I read it over and it is what’s called for. (In reply to Andy VanWagoner from comment #12) > for the default Number.prototype.toLocaleString Right, my comment was not about the default function. > Arguments { 0: undefined, 1: undefined } Good, Firefox implementation matches.