Implement `String.prototype.codePointAt` and `String.fromCodePoint`

Implement `String.prototype.codePointAt` and `String.fromCodePoint`

Contributed by Mathias Bynens <mathiasb@opera.com>.

TEST=mjsunit/harmony
BUG=v8:2840
LOG=Y

[mathias, 2013-09-23 18:01:43]

test/mjsunit/string-fromcodepoint.js is a completely new file. I’m not sure why
the diff shows up like that.

[mathias, 2013-09-23 19:25:48]

On 2013/09/23 18:01:43, mathias wrote:
> test/mjsunit/string-fromcodepoint.js is a completely new file. I’m not sure
why
> the diff shows up like that.

Ah, `git cl upload --similarity=70` seemed to avoid the issue.

[mstarzinger, 2013-09-24 08:03:53]

One high level comment: This needs to go behind the --harmony flag since it is
an experimental ES6 feature. Adding Dmitry for the procedural side of things.

[mathias, 2013-09-24 08:58:22]

On 2013/09/24 08:03:53, mstarzinger wrote:
> One high level comment: This needs to go behind the --harmony flag since it is
> an experimental ES6 feature.

Patch updated accordingly.

[dslomov, 2013-09-24 09:37:38]

On 2013/09/24 08:58:22, mathias wrote:
> On 2013/09/24 08:03:53, mstarzinger wrote:
> > One high level comment: This needs to go behind the --harmony flag since it
is
> > an experimental ES6 feature.
> 
> Patch updated accordingly.

Before we get to land this, please follow the
https://developers.google.com/v8/launchprocess.
In particular, send the intent-to-implement mail to v8-users@googlegroups.com.
For this small feature, you do not need a design doc, but please specify in your
mail what the status is in other browsers, and especially what is the situation
re ES6 standard/community reaction. 

Just to give you a piece of my mind: reading this thread:
http://esdiscuss.org/topic/code-points-vs-unicode-scalar-values, it is not at
all obvious to me that it is already a good time to implement this: the design
is a controversy, and implementation feedback from the engines is not really
required to resolve the controversy (the design can easily be tried with a
polyfill with no expressiveness or performance issues). These kinds of features
(whole lot of tricky edge cases that need careful testing, but without large
implementation changes) should really be put into the engine at a later stage.
Otherwise we will have to update the code and the tests later when spec changes
its mind about the treatment of this or that Unicode character, and that's a
pain.

I wouldn't rush this patch in, but if you think otherwise feel free to reach out
to us in intent-to-implement e-mail.

[mathias, 2013-09-24 10:21:24]

On 2013/09/24 09:37:38, dslomov wrote:
> Before we get to land this, please follow the
> https://developers.google.com/v8/launchprocess.
> In particular, send the intent-to-implement mail to
mailto:v8-users@googlegroups.com.

https://groups.google.com/d/msg/v8-users/znHqssyF0_0/nSwa8i_pWs4J

[Erik Arvidsson, 2013-09-24 15:47:17]

From http://esdiscuss.org/topic/code-points-vs-unicode-scalar-values it is
not clear that there was a consensus that it was the right thing to do to
add codePointFrom and codePointAt. I know this is not very like me but I
believe we should wait until this has been resolved in es-discuss and/or
TC39. Mathias, would you mind starting a new thread on es-discuss since the
original thread diverted to JSON.stringify which we did discuss at the last
face to face meeting.


On Tue, Sep 24, 2013 at 3:21 AM, <mathias@qiwi.be> wrote:

> On 2013/09/24 09:37:38, dslomov wrote:
>
>> Before we get to land this, please follow the
>>
https://developers.google.com/**v8/launchprocess<https://developers.google.co...
>> .
>> In particular, send the intent-to-implement mail to
>>
> mailto:v8-users@googlegroups.**com <v8-users@googlegroups.com>.
>
>
https://groups.google.com/d/**msg/v8-users/znHqssyF0_0/**nSwa8i_pWs4J<https:/...
>
>
https://codereview.appspot.**com/13816046/<https://codereview.appspot.com/138...
>



-- 
erik

[mathias, 2013-09-26 13:32:41]

On 2013/09/24 15:47:17, Erik Arvidsson wrote:
> […] Mathias, would you mind starting a new thread on es-discuss since the
> original thread diverted to JSON.stringify which we did discuss at the last
> face to face meeting.

Sure:
http://esdiscuss.org/topic/on-string-prototype-codepointat-and-string-fromcod...

[mathias, 2014-01-28 11:16:57]

Rebased and updated to use the new `CHECK_OBJECT_COERCIBLE` macro.

[arv, 2014-06-19 15:29:56]

I wanted to bring this back to life.

I feel that this is stable enough now and the spec is set in stone.

https://codereview.appspot.com/13816046/diff/63001/src/harmony-string.js
File src/harmony-string.js (right):

https://codereview.appspot.com/13816046/diff/63001/src/harmony-string.js#newc...
src/harmony-string.js:149: pos = TO_INTEGER(pos);
To_INTEGER already does %_IsSmi.

https://codereview.appspot.com/13816046/diff/63001/src/harmony-string.js#newc...
src/harmony-string.js:152: return void 0;
Use UNDEFINED instead?

https://codereview.appspot.com/13816046/diff/63001/test/mjsunit/harmony/strin...
File test/mjsunit/harmony/string-codepointat.js (right):

https://codereview.appspot.com/13816046/diff/63001/test/mjsunit/harmony/strin...
test/mjsunit/harmony/string-codepointat.js:85: toString: function() { return
"abc"; }
maybe add a test that toString is only called once?

https://codereview.appspot.com/13816046/diff/63001/test/mjsunit/harmony/strin...
test/mjsunit/harmony/string-codepointat.js:89:
String.prototype.codePointAt.apply(undefined);
These are duplicate tests of the same with call. You can remove them.

https://codereview.appspot.com/13816046/diff/63001/test/mjsunit/harmony/strin...
File test/mjsunit/harmony/string-fromcodepoint.js (right):

https://codereview.appspot.com/13816046/diff/63001/test/mjsunit/harmony/strin...
test/mjsunit/harmony/string-fromcodepoint.js:19: String.fromCodePoint(0x1D306,
0x61, 0x1D307),
Indent 4 spaces when line breaking a statement.

https://codereview.appspot.com/13816046/diff/63001/test/mjsunit/harmony/strin...
test/mjsunit/harmony/string-fromcodepoint.js:21: );
); at the end of previous line:

assertEquals(
    String.fromCodePoint(0x1D306, 0x61, 0x1D307),
    "\uD834\uDF06a\uD834\uDF07");

https://codereview.appspot.com/13816046/diff/63001/test/mjsunit/harmony/strin...
test/mjsunit/harmony/string-fromcodepoint.js:39: 
Maybe add a test where valueOf throws?

https://codereview.appspot.com/13816046/diff/63001/test/mjsunit/harmony/strin...
test/mjsunit/harmony/string-fromcodepoint.js:39: 
Maybe add a test that ensures that valueOf is called only once?

[mathias, 2014-06-19 21:48:57]

On 2014/06/19 15:29:56, arv wrote:
> I wanted to bring this back to life.
> 
> I feel that this is stable enough now and the spec is set in stone.

Glad to hear! I’ve updated the patch as per your feedback.

[arv, 2014-06-19 22:24:44]

https://codereview.appspot.com/13816046/diff/83001/test/mjsunit/harmony/strin...
File test/mjsunit/harmony/string-fromcodepoint.js (right):

https://codereview.appspot.com/13816046/diff/83001/test/mjsunit/harmony/strin...
test/mjsunit/harmony/string-fromcodepoint.js:47: }), "a");
Assert tmp here too (and in the other file)

[mathias, 2014-06-20 05:23:59]

On 2014/06/19 22:24:44, arv wrote:
> Assert tmp here too (and in the other file)

Done.

[mathias, 2014-06-20 05:25:28]

https://codereview.appspot.com/13816046/diff/103001/src/harmony-string.js
File src/harmony-string.js (right):

https://codereview.appspot.com/13816046/diff/103001/src/harmony-string.js#new...
src/harmony-string.js:12: // var ArrayPushBuiltin = $Array.prototype.push;
This can be removed now that we no longer use it.

[mathias, 2014-06-20 05:28:12]

On 2014/06/20 05:25:28, mathias wrote:
> This can be removed now that we no longer use it.

Done.

[arv, 2014-06-20 13:57:41]

LGTM

Michael, can you look at this too?

[arv, 2014-07-09 14:12:05]

Dmitry, Andreas. Care to take a look?

[rossberg, 2014-07-09 14:16:17]

On 2014/07/09 14:12:05, arv wrote:
> Dmitry, Andreas. Care to take a look?

yangguo@ should look at this (somehow I can't edit the issue to add him,
though).

[arv, 2014-07-09 14:19:04]

Yang, PTAL

[yangguo, 2014-07-15 14:52:42]

Haven't had much time to go over the spec yet, but first thing I noticed...

Also, why is this not on codereview.chromium.org, and why is the base URL
referring to the git mirror?

https://codereview.appspot.com/13816046/diff/123001/src/harmony-string.js
File src/harmony-string.js (right):

https://codereview.appspot.com/13816046/diff/123001/src/harmony-string.js#new...
src/harmony-string.js:146: function StringFromCodePoint() {
The latest draft (from 5-22-2014) actually specifies the function length to be
1. That makes sense, since that would be consistent to String.fromCharCode

Also update the test case please.

https://codereview.appspot.com/13816046/diff/123001/test/mjsunit/harmony/stri...
File test/mjsunit/harmony/string-fromcodepoint.js (right):

https://codereview.appspot.com/13816046/diff/123001/test/mjsunit/harmony/stri...
test/mjsunit/harmony/string-fromcodepoint.js:8: //
https://github.com/mathiasbynens/String.fromCodePoint/blob/master/tests/tests.js
would be nice if we didn't violate the 80-char limit here.

[mathias, 2014-07-15 15:04:39]

Updated as per Yang’s feedback. PTAL

[arv, 2014-07-15 15:47:42]

FYI

https://codereview.appspot.com/13816046/diff/133001/src/harmony-string.js
File src/harmony-string.js (right):

https://codereview.appspot.com/13816046/diff/133001/src/harmony-string.js#new...
src/harmony-string.js:146: function StringFromCodePoint(_) {  // length == 1
Use %FunctionSetLength instead?

https://codereview.appspot.com/13816046/diff/133001/src/harmony-string.js#new...
src/harmony-string.js:188: "codePointAt", StringCodePointAt
Maybe sort these?

[mathias, 2014-07-15 16:08:08]

On 2014/07/15 at 15:47:42, arv wrote:
>
https://codereview.appspot.com/13816046/diff/133001/src/harmony-string.js#new...
> src/harmony-string.js:146: function StringFromCodePoint(_) {  // length == 1
> Use %FunctionSetLength instead?

FWIW, I did it this way because that’s how it’s done for `String.fromCharCode`
too. Updated the patch to use `%FunctionSetLength`.
 
>
https://codereview.appspot.com/13816046/diff/133001/src/harmony-string.js#new...
> src/harmony-string.js:188: "codePointAt", StringCodePointAt
> Maybe sort these?

Done.

[yangguo, 2014-07-16 13:06:31]

Some comments. Otherwise LGTM.

https://codereview.appspot.com/13816046/diff/153001/src/harmony-string.js
File src/harmony-string.js (right):

https://codereview.appspot.com/13816046/diff/153001/src/harmony-string.js#new...
src/harmony-string.js:141: return (first - 0xD800) * 0x400 + second - 0xDC00 +
0x10000;
I don't think unoptimized code does any constant folding, so at least for
unoptimized code it would make sense to manually fold -0xDC00+0x10000.

https://codereview.appspot.com/13816046/diff/153001/src/harmony-string.js#new...
src/harmony-string.js:160: result += StringFromCharCode(code);
Since we already did all the checks and conversions, it would be more performant
here to just call %_StringCharFromCode.

https://codereview.appspot.com/13816046/diff/153001/src/harmony-string.js#new...
src/harmony-string.js:164: (code - 0x10000) % 0x400 + 0xDC00
Not entirely sure whether this is the case for optimized code, but at least get
alot nicer unoptimized code if we used right shift instead of div and floor, and
bit-masking instead of modulo.

https://codereview.appspot.com/13816046/diff/153001/src/harmony-string.js#new...
src/harmony-string.js:170: %FunctionSetLength(StringFromCodePoint, 1);
I actually prefer the previous version, being consistent to StringFromCharCode.

[rossberg_google.com, 2014-07-16 13:32:11]

On 16 July 2014 15:06,  <yangguo@chromium.org> wrote:
> https://codereview.appspot.com/13816046/diff/153001/src/harmony-string.js
> File src/harmony-string.js (right):
>
>
https://codereview.appspot.com/13816046/diff/153001/src/harmony-string.js#new...
> src/harmony-string.js:141: return (first - 0xD800) * 0x400 + second -
> 0xDC00 + 0x10000;
> I don't think unoptimized code does any constant folding, so at least
> for unoptimized code it would make sense to manually fold
> -0xDC00+0x10000.

DBC: Actually, the parser already does simple constant folding like that.

[mathias, 2014-07-17 21:10:48]

On 2014/07/16 at 13:06:31, yangguo wrote:
> Some comments. Otherwise LGTM.

Thanks for the review! Patch updated.

[yangguo, 2014-07-21 08:34:33]

On 2014/07/17 21:10:48, mathias wrote:
> On 2014/07/16 at 13:06:31, yangguo wrote:
> > Some comments. Otherwise LGTM.
> 
> Thanks for the review! Patch updated.

I created a copy of this CL here: https://codereview.chromium.org/406863003/

Please use codereview.chromium.org for the future, and use
https://v8.googlecode.com/svn/branches/bleeding_edge as base url.