Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
996223003:
[es6] support template literals after MemberExpression (Closed)
|
Created:
5 years, 9 months ago by caitp (gmail) Modified:
5 years, 9 months ago CC:
v8-dev Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[es6] support template literals after MemberExpression
BUG=v8:3958, 450942
LOG=N
R=arv@chromium.org
Committed: https://crrev.com/1aae3a1c89c2c4283c8e611930ce74f49d6cd9b1
Cr-Commit-Position: refs/heads/master@{#27159}
Patch Set 1 #Patch Set 2 : Remove duplicated code, simplify test code #Patch Set 3 : Delete meaningless edit #
Total comments: 2
Patch Set 4 : Get rid of handling in LeftHandSideExpression, add comment #Patch Set 5 : Fix tagged template NewExpression case #
Total comments: 6
Patch Set 6 : New tests, delete unneeded code, some comments #
Messages
Total messages: 27 (10 generated)
caitpotter88@gmail.com changed reviewers: + marja@chromium.org
PTAL Things on the TODO list: - Don't duplicate code between ParseLeftHandSideExpression and ParseMemberExpressionContinuation - More tests?
LGTM https://codereview.chromium.org/996223003/diff/40001/src/scanner.h File src/scanner.h (right): https://codereview.chromium.org/996223003/diff/40001/src/scanner.h#newcode723 src/scanner.h:723: LiteralBuffer raw_literal_buffer2_; Ah, I remember this... Silly me thinking that you could not have 2 template literals after each other.
On 2015/03/11 18:25:11, arv wrote: > LGTM > > https://codereview.chromium.org/996223003/diff/40001/src/scanner.h > File src/scanner.h (right): > > https://codereview.chromium.org/996223003/diff/40001/src/scanner.h#newcode723 > src/scanner.h:723: LiteralBuffer raw_literal_buffer2_; > Ah, I remember this... Silly me thinking that you could not have 2 template > literals after each other. Thanks for the review
caitpotter88@gmail.com changed reviewers: - dslomov@chromium.org, marja@chromium.org
The CQ bit was checked by caitpotter88@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/996223003/40001
The CQ bit was unchecked by caitpotter88@gmail.com
https://codereview.chromium.org/996223003/diff/40001/src/preparser.h File src/preparser.h (right): https://codereview.chromium.org/996223003/diff/40001/src/preparser.h#newcode2812 src/preparser.h:2812: // ('[' Expression ']' | '.' Identifier)* update this comment?
On 2015/03/11 18:48:54, arv wrote: > https://codereview.chromium.org/996223003/diff/40001/src/preparser.h > File src/preparser.h (right): > > https://codereview.chromium.org/996223003/diff/40001/src/preparser.h#newcode2812 > src/preparser.h:2812: // ('[' Expression ']' | '.' Identifier)* > update this comment? Done --- I think it's right now... Template is only parsed as PrimaryExpression or as part of a MemberExpression
The CQ bit was checked by caitpotter88@gmail.com
The patchset sent to the CQ was uploaded after l-g-t-m from arv@chromium.org Link to the patchset: https://codereview.chromium.org/996223003/#ps60001 (title: "Get rid of handling in LeftHandSideExpression, add comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/996223003/60001
The CQ bit was unchecked by caitpotter88@gmail.com
New test for new tag`...` `...` `...`, behaviour appears to match Nightly, but reading the spec it's really confusing what __should__ happen. I think sorting out the duplicate code in ParseLeftHandSideExpression can wait for a different CL
caitpotter88@gmail.com changed reviewers: + marja@chromium.org
On 2015/03/11 20:22:21, caitp wrote: > New test for new tag`...` `...` `...`, behaviour appears to match Nightly, but > reading the spec it's really confusing what __should__ happen. > > I think sorting out the duplicate code in ParseLeftHandSideExpression can wait > for a different CL marja@ do you have a comment on the duplicate code in ParseLeftHandSideExpression, and generally on how this adheres to the spec? I think it's matching SpiderMonkey, in all the cases I've tried
LGTM Re: spec, I think your interpretation is correct (as far as I can tell after a 10 min reading session). Which duplicate code are you talking about, since you removed the template token handling from ParseLeftHandSideExpression? In any case, this CL doesn't seem to be making anything worse. https://codereview.chromium.org/996223003/diff/80001/src/preparser.h File src/preparser.h (right): https://codereview.chromium.org/996223003/diff/80001/src/preparser.h#newcode2713 src/preparser.h:2713: // new tag`a` -> new(tag`a`) Pls move this comment up in the comment block, it's more discoverable there. https://codereview.chromium.org/996223003/diff/80001/src/preparser.h#newcode2806 src/preparser.h:2806: // Parses this part of MemberExpression: You probably need to change the comment in ParseMemberExpression too https://codereview.chromium.org/996223003/diff/80001/test/mjsunit/harmony/tem... File test/mjsunit/harmony/templates.js (right): https://codereview.chromium.org/996223003/diff/80001/test/mjsunit/harmony/tem... test/mjsunit/harmony/templates.js:547: this.x = x === void 0 ? null : x; What's the added value of storing this.x, since we already log.push(x) ?
https://codereview.chromium.org/996223003/diff/80001/src/preparser.h File src/preparser.h (right): https://codereview.chromium.org/996223003/diff/80001/src/preparser.h#newcode2713 src/preparser.h:2713: // new tag`a` -> new(tag`a`) On 2015/03/12 11:43:35, marja wrote: > Pls move this comment up in the comment block, it's more discoverable there. this whole block seems to be unneeded, since ParseMemberExpressionWithNewPrefixes parses the templates on its own. Just removing from parseLHS seems to have been enough https://codereview.chromium.org/996223003/diff/80001/test/mjsunit/harmony/tem... File test/mjsunit/harmony/templates.js (right): https://codereview.chromium.org/996223003/diff/80001/test/mjsunit/harmony/tem... test/mjsunit/harmony/templates.js:547: this.x = x === void 0 ? null : x; On 2015/03/12 11:43:35, marja wrote: > What's the added value of storing this.x, since we already log.push(x) ? it's distinct because only the one which is actually called as a constructor will have a valid "this"
The CQ bit was checked by caitpotter88@gmail.com
The patchset sent to the CQ was uploaded after l-g-t-m from arv@chromium.org, marja@chromium.org Link to the patchset: https://codereview.chromium.org/996223003/#ps100001 (title: "New tests, delete unneeded code, some comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/996223003/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/1aae3a1c89c2c4283c8e611930ce74f49d6cd9b1 Cr-Commit-Position: refs/heads/master@{#27159}
Message was sent while issue was closed.
LGTM https://codereview.chromium.org/996223003/diff/80001/test/mjsunit/harmony/tem... File test/mjsunit/harmony/templates.js (right): https://codereview.chromium.org/996223003/diff/80001/test/mjsunit/harmony/tem... test/mjsunit/harmony/templates.js:579: })(); One more thing we could test would be: new tag `x` `y` ('z') which I believe should be treated as new (tag `x` `y`)('z') ie construct (tag `x` `y`) with 'y' as a param.
Message was sent while issue was closed.
On 2015/03/12 15:20:03, arv wrote: > LGTM > > https://codereview.chromium.org/996223003/diff/80001/test/mjsunit/harmony/tem... > File test/mjsunit/harmony/templates.js (right): > > https://codereview.chromium.org/996223003/diff/80001/test/mjsunit/harmony/tem... > test/mjsunit/harmony/templates.js:579: })(); > One more thing we could test would be: > > new tag `x` `y` ('z') > > which I believe should be treated as > > new (tag `x` `y`)('z') > > ie construct (tag `x` `y`) with 'y' as a param. You mean with 'z' as a param? I added this test in the checked in commit, it matches Firefox so I think that's good
Message was sent while issue was closed.
On 2015/03/12 15:21:37, caitp wrote: > On 2015/03/12 15:20:03, arv wrote: > > LGTM > > > > > https://codereview.chromium.org/996223003/diff/80001/test/mjsunit/harmony/tem... > > File test/mjsunit/harmony/templates.js (right): > > > > > https://codereview.chromium.org/996223003/diff/80001/test/mjsunit/harmony/tem... > > test/mjsunit/harmony/templates.js:579: })(); > > One more thing we could test would be: > > > > new tag `x` `y` ('z') > > > > which I believe should be treated as > > > > new (tag `x` `y`)('z') > > > > ie construct (tag `x` `y`) with 'y' as a param. > > You mean with 'z' as a param? I added this test in the checked in commit, it > matches Firefox so I think that's good Oops. yeah 'z' |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
