Implement ES6 String.raw behind --harmony-templates

Implement ES6 String.raw behind --harmony-templates

BUG=

Committed: https://chromium.googlesource.com/v8/v8/+/e829bcd73b2fa1c0bbe3da36a723fa77c734a05c

[caitp (gmail), 2014-11-14 21:06:52]

A quick addition to the String object which makes more sense now that templates
are implemented-ish

[arv (Not doing code reviews), 2014-11-14 22:54:24]

arv@chromium.org changed reviewers:
+ arv@chromium.org

[arv (Not doing code reviews), 2014-11-14 22:54:25]

I think it would make more sense to put this under --harmony-templates

https://codereview.chromium.org/731573004/diff/20001/src/harmony-string.js
File src/harmony-string.js (right):

https://codereview.chromium.org/731573004/diff/20001/src/harmony-string.js#ne...
src/harmony-string.js:177: var substitutions =
$Array.prototype.slice.call(arguments, 1);
This is not safe. Use a c-style for loop using %_ArgumentsLength and %_Arguments
instead.

https://codereview.chromium.org/731573004/diff/20001/src/harmony-string.js#ne...
src/harmony-string.js:187: while (true) {
for (var nextIndex = 0; ; nextIndex++) {

https://codereview.chromium.org/731573004/diff/20001/src/harmony-string.js#ne...
src/harmony-string.js:188: var next = ToString(raw[nextIndex]);
Don't we know that these are all strings?

https://codereview.chromium.org/731573004/diff/20001/src/harmony-string.js#ne...
src/harmony-string.js:189: result = result + next;
+=

https://codereview.chromium.org/731573004/diff/20001/test/mjsunit/harmony/str...
File test/mjsunit/harmony/string-raw.js (right):

https://codereview.chromium.org/731573004/diff/20001/test/mjsunit/harmony/str...
test/mjsunit/harmony/string-raw.js:111: })();
You know I was going to ask for this...

Can you add some tests with accessors so we can ensure the execution order is
correct.

Also, make some of these accessors throw in strange placse.

And also arrays with holes/crap on the prototype.

[caitp (gmail), 2014-11-14 23:29:58]

I felt like it made more sense under --harmony-strings because it doesn't really
depend on template literals, but I've changed it to be behind
--harmony-templates anyways.

https://codereview.chromium.org/731573004/diff/20001/src/harmony-string.js
File src/harmony-string.js (right):

https://codereview.chromium.org/731573004/diff/20001/src/harmony-string.js#ne...
src/harmony-string.js:177: var substitutions =
$Array.prototype.slice.call(arguments, 1);
On 2014/11/14 22:54:25, arv wrote:
> This is not safe. Use a c-style for loop using %_ArgumentsLength and
%_Arguments
> instead.

Done.

https://codereview.chromium.org/731573004/diff/20001/src/harmony-string.js#ne...
src/harmony-string.js:187: while (true) {
On 2014/11/14 22:54:25, arv wrote:
> for (var nextIndex = 0; ; nextIndex++) {

I've fixed this slightly differently, so it looks a bit different from the spec,
but I think it works better given the use of the arguments[] index and it may
possibly generate slightly better code.

https://codereview.chromium.org/731573004/diff/20001/src/harmony-string.js#ne...
src/harmony-string.js:188: var next = ToString(raw[nextIndex]);
On 2014/11/14 22:54:25, arv wrote:
> Don't we know that these are all strings?

The algorithm specifically calls for ToString(), which would be expected to
throw in the usual cases where it needs to through, I believe.

I'm not positive that the implementation of ToString() in runtime.js is an exact
match with the current draft's version, though.

https://codereview.chromium.org/731573004/diff/20001/src/harmony-string.js#ne...
src/harmony-string.js:189: result = result + next;
On 2014/11/14 22:54:25, arv wrote:
> +=

Acknowledged.

https://codereview.chromium.org/731573004/diff/20001/test/mjsunit/harmony/str...
File test/mjsunit/harmony/string-raw.js (right):

https://codereview.chromium.org/731573004/diff/20001/test/mjsunit/harmony/str...
test/mjsunit/harmony/string-raw.js:111: })();
On 2014/11/14 22:54:25, arv wrote:
> You know I was going to ask for this...
> 
> Can you add some tests with accessors so we can ensure the execution order is
> correct.
> 
> Also, make some of these accessors throw in strange placse.
> 
> And also arrays with holes/crap on the prototype.

Great ideas =) I'll add these shortly.

[arv (Not doing code reviews), 2014-11-14 23:42:18]

https://codereview.chromium.org/731573004/diff/20001/src/harmony-string.js
File src/harmony-string.js (right):

https://codereview.chromium.org/731573004/diff/20001/src/harmony-string.js#ne...
src/harmony-string.js:187: while (true) {
On 2014/11/14 23:29:58, caitp wrote:
> On 2014/11/14 22:54:25, arv wrote:
> > for (var nextIndex = 0; ; nextIndex++) {
> 
> I've fixed this slightly differently, so it looks a bit different from the
spec,
> but I think it works better given the use of the arguments[] index and it may
> possibly generate slightly better code.

I like your new code better. Easier to grok.

https://codereview.chromium.org/731573004/diff/20001/src/harmony-string.js#ne...
src/harmony-string.js:188: var next = ToString(raw[nextIndex]);
On 2014/11/14 23:29:58, caitp wrote:
> On 2014/11/14 22:54:25, arv wrote:
> > Don't we know that these are all strings?
> 
> The algorithm specifically calls for ToString(), which would be expected to
> throw in the usual cases where it needs to through, I believe.
> 
> I'm not positive that the implementation of ToString() in runtime.js is an
exact
> match with the current draft's version, though.

I guess it doesn't hurt to call ToString on a string and you are right that raw
might not actually be strings if someone uses String.raw on their own object.

https://codereview.chromium.org/731573004/diff/20001/test/mjsunit/harmony/str...
File test/mjsunit/harmony/string-raw.js (right):

https://codereview.chromium.org/731573004/diff/20001/test/mjsunit/harmony/str...
test/mjsunit/harmony/string-raw.js:111: })();
On 2014/11/14 23:29:58, caitp wrote:
> On 2014/11/14 22:54:25, arv wrote:
> > You know I was going to ask for this...
> > 
> > Can you add some tests with accessors so we can ensure the execution order
is
> > correct.
> > 
> > Also, make some of these accessors throw in strange placse.
> > 
> > And also arrays with holes/crap on the prototype.
> 
> Great ideas =) I'll add these shortly.

Also some toString objects (with and without exceptions)

[caitp (gmail), 2014-11-15 01:33:40]

Patchset #4 (id:60001) has been deleted

[caitp (gmail), 2014-11-18 15:38:56]

On 2014/11/14 23:42:18, arv wrote:
> https://codereview.chromium.org/731573004/diff/20001/src/harmony-string.js
> File src/harmony-string.js (right):
> 
>
https://codereview.chromium.org/731573004/diff/20001/src/harmony-string.js#ne...
> src/harmony-string.js:187: while (true) {
> On 2014/11/14 23:29:58, caitp wrote:
> > On 2014/11/14 22:54:25, arv wrote:
> > > for (var nextIndex = 0; ; nextIndex++) {
> > 
> > I've fixed this slightly differently, so it looks a bit different from the
> spec,
> > but I think it works better given the use of the arguments[] index and it
may
> > possibly generate slightly better code.
> 
> I like your new code better. Easier to grok.
> 
>
https://codereview.chromium.org/731573004/diff/20001/src/harmony-string.js#ne...
> src/harmony-string.js:188: var next = ToString(raw[nextIndex]);
> On 2014/11/14 23:29:58, caitp wrote:
> > On 2014/11/14 22:54:25, arv wrote:
> > > Don't we know that these are all strings?
> > 
> > The algorithm specifically calls for ToString(), which would be expected to
> > throw in the usual cases where it needs to through, I believe.
> > 
> > I'm not positive that the implementation of ToString() in runtime.js is an
> exact
> > match with the current draft's version, though.
> 
> I guess it doesn't hurt to call ToString on a string and you are right that
raw
> might not actually be strings if someone uses String.raw on their own object.
> 
>
https://codereview.chromium.org/731573004/diff/20001/test/mjsunit/harmony/str...
> File test/mjsunit/harmony/string-raw.js (right):
> 
>
https://codereview.chromium.org/731573004/diff/20001/test/mjsunit/harmony/str...
> test/mjsunit/harmony/string-raw.js:111: })();
> On 2014/11/14 23:29:58, caitp wrote:
> > On 2014/11/14 22:54:25, arv wrote:
> > > You know I was going to ask for this...
> > > 
> > > Can you add some tests with accessors so we can ensure the execution order
> is
> > > correct.
> > > 
> > > Also, make some of these accessors throw in strange placse.
> > > 
> > > And also arrays with holes/crap on the prototype.
> > 
> > Great ideas =) I'll add these shortly.
> 
> Also some toString objects (with and without exceptions)

How does that look? (non-throwing toString and valueOf tests are precent in the
literal tests)

[caitp (gmail), 2014-11-18 16:58:32]

caitpotter88@gmail.com changed reviewers:
+ rossberg@chromium.org

[caitp (gmail), 2014-11-18 18:22:57]

caitpotter88@gmail.com changed reviewers:
+ dslomov@chromium.org

[arv (Not doing code reviews), 2014-11-18 19:05:28]

LGTM, just some style nits. You will need an owner to approve this too.

https://codereview.chromium.org/731573004/diff/100001/src/harmony-templates.js
File src/harmony-templates.js (right):

https://codereview.chromium.org/731573004/diff/100001/src/harmony-templates.j...
src/harmony-templates.js:19: // TODO(*): Use rest parameters when implemented
We always use a name here. The name should be of the person to ask questions to,
not necessarily the person that needs to fix the issue.

https://codereview.chromium.org/731573004/diff/100001/src/harmony-templates.j...
src/harmony-templates.js:39: //
-------------------------------------------------------------------
skip this line

https://codereview.chromium.org/731573004/diff/100001/test/mjsunit/harmony/st...
File test/mjsunit/harmony/string-raw.js (right):

https://codereview.chromium.org/731573004/diff/100001/test/mjsunit/harmony/st...
test/mjsunit/harmony/string-raw.js:106: callSiteObj.raw = {length: 2, '0':"a",
'1':"b", '2':"c"};
ws after :

https://codereview.chromium.org/731573004/diff/100001/test/mjsunit/harmony/st...
test/mjsunit/harmony/string-raw.js:106: callSiteObj.raw = {length: 2, '0':"a",
'1':"b", '2':"c"};
don't mix and match ' and ". Stick to one.

https://codereview.chromium.org/731573004/diff/100001/test/mjsunit/harmony/st...
test/mjsunit/harmony/string-raw.js:250: [1,3,5].forEach(function(v, i) {
ws after comma

[caitp (gmail), 2014-11-18 19:11:54]

Done and done!

https://codereview.chromium.org/731573004/diff/100001/src/harmony-templates.js
File src/harmony-templates.js (right):

https://codereview.chromium.org/731573004/diff/100001/src/harmony-templates.j...
src/harmony-templates.js:19: // TODO(*): Use rest parameters when implemented
On 2014/11/18 19:05:27, arv wrote:
> We always use a name here. The name should be of the person to ask questions
to,
> not necessarily the person that needs to fix the issue.

Done.

https://codereview.chromium.org/731573004/diff/100001/src/harmony-templates.j...
src/harmony-templates.js:39: //
-------------------------------------------------------------------
On 2014/11/18 19:05:27, arv wrote:
> skip this line

Done.

https://codereview.chromium.org/731573004/diff/100001/test/mjsunit/harmony/st...
File test/mjsunit/harmony/string-raw.js (right):

https://codereview.chromium.org/731573004/diff/100001/test/mjsunit/harmony/st...
test/mjsunit/harmony/string-raw.js:106: callSiteObj.raw = {length: 2, '0':"a",
'1':"b", '2':"c"};
On 2014/11/18 19:05:27, arv wrote:
> don't mix and match ' and ". Stick to one.

Done.

https://codereview.chromium.org/731573004/diff/100001/test/mjsunit/harmony/st...
test/mjsunit/harmony/string-raw.js:106: callSiteObj.raw = {length: 2, '0':"a",
'1':"b", '2':"c"};
On 2014/11/18 19:05:27, arv wrote:
> ws after :

Done.

https://codereview.chromium.org/731573004/diff/100001/test/mjsunit/harmony/st...
test/mjsunit/harmony/string-raw.js:250: [1,3,5].forEach(function(v, i) {
On 2014/11/18 19:05:27, arv wrote:
> ws after comma

Done.

[Dmitry Lomov (no reviews), 2014-11-18 20:43:37]

lgtm

[caitp (gmail), 2014-11-18 21:08:05]

The CQ bit was checked by caitpotter88@gmail.com

[commit-bot: I haz the power, 2014-11-18 21:09:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/731573004/120001

[commit-bot: I haz the power, 2014-11-19 04:44:43]

Committed patchset #6 (id:120001)