Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
663683006:
Implement ES6 Template Literals (Closed)
|
Created:
6 years, 1 month ago by caitp (gmail) Modified:
6 years ago CC:
v8-dev, adamk Project:
v8 Visibility:
Public. |
DescriptionImplement ES6 Template Literals
BUG=v8:3230
Committed: https://chromium.googlesource.com/v8/v8/+/edc225f945978615c5cb327eef341c6e8d2f16e5
Patch Set 1 #
Total comments: 4
Patch Set 2 : Remove Scanner::Mode, among other things #Patch Set 3 : Make untagged template literals actually work #Patch Set 4 : Implement tagged template literals #Patch Set 5 : Add test cases for harmony-templates #
Total comments: 2
Patch Set 6 : Fix scanner bugs, add more tests #Patch Set 7 : Prevent fall-through to template token handlers #
Total comments: 1
Patch Set 8 : Remove TemplateLiteral AST node, do it all in parsing #
Total comments: 9
Patch Set 9 : some extra parsing tests #Patch Set 10 : More tests again #
Total comments: 10
Patch Set 11 : Add comments describing the parsing and scanning of TemplateLiterals #
Total comments: 1
Patch Set 12 : Tiny fixups #
Total comments: 9
Patch Set 13 : More comments + nits #Patch Set 14 : Don't record raw literals #Patch Set 15 : Remove kTemplateLiteral bailout-reason #
Total comments: 9
Patch Set 16 : Additional TRV tests #Patch Set 17 : Fix issue with line-continuation, nits #
Total comments: 15
Patch Set 18 : Rename temporary variables for readability #
Total comments: 1
Patch Set 19 : ensure scanner flag is initialized to false #
Total comments: 2
Patch Set 20 : Reorganize generation of raw template strings, slightly #Patch Set 21 : Report unterminated template literals error #Patch Set 22 : Simplify GetTemplateCallSite() #
Total comments: 5
Patch Set 23 : Add more redundant and thorough mjsunit tests #
Total comments: 6
Patch Set 24 : Better callSiteObj shape-testing, move tests to harmony/templates.js #
Total comments: 1
Patch Set 25 : Better error reporting + use Array.isArray() rather than instanceof #Patch Set 26 : Fix trailing space #Patch Set 27 : Even better static error reporting #
Total comments: 9
Patch Set 28 : Nits + extra parsing tests (pre clang-format) #Patch Set 29 : Clang-formatted #
Total comments: 1
Patch Set 30 : Add harmony-templates to BUILD.gn #Patch Set 31 : Get rid of nested parentheses #Patch Set 32 : Fix failing DCHECKs in parser.cc and preparser.h #Patch Set 33 : Rebased against https://chromium.googlesource.com/v8/v8.git/master #
Total comments: 3
Messages
Total messages: 89 (11 generated)
Thought I'd have a go at it. It's not done, but parsing sort of works at least.
arv@chromium.org changed reviewers: + arv@chromium.org
Neat. https://codereview.chromium.org/663683006/diff/1/src/bailout-reason.h File src/bailout-reason.h (right): https://codereview.chromium.org/663683006/diff/1/src/bailout-reason.h#newcode326 src/bailout-reason.h:326: V(kTemplateLiteral, "Template Literal") Keep in alphabetical order. https://codereview.chromium.org/663683006/diff/1/src/scanner.h File src/scanner.h (right): https://codereview.chromium.org/663683006/diff/1/src/scanner.h#newcode363 src/scanner.h:363: Token::Value Next(Mode mode = None); I'm not sure you want to use a mode here. Once you are in a template literal you can just call the right function, similar to how we handle regular expressions.
https://codereview.chromium.org/663683006/diff/1/src/bailout-reason.h File src/bailout-reason.h (right): https://codereview.chromium.org/663683006/diff/1/src/bailout-reason.h#newcode326 src/bailout-reason.h:326: V(kTemplateLiteral, "Template Literal") On 2014/10/27 18:14:06, arv wrote: > Keep in alphabetical order. Done. https://codereview.chromium.org/663683006/diff/1/src/scanner.h File src/scanner.h (right): https://codereview.chromium.org/663683006/diff/1/src/scanner.h#newcode363 src/scanner.h:363: Token::Value Next(Mode mode = None); On 2014/10/27 18:14:06, arv wrote: > I'm not sure you want to use a mode here. Once you are in a template literal you > can just call the right function, similar to how we handle regular expressions. Done. SpiderMonkey uses the mode strategy for this, but it's true that it's not really needed, so it's gone.
caitpotter88@gmail.com changed reviewers: + rossberg@chromium.org
Before this CL can land, I think we need a clear design for implementation of this feature. In particular: 1. What the code generation would look like, on a high level? 2. How will we cache template call sites? It is my guess that after we figure out 1 and 2, we will realize that we do not actually need new AST nodes for template strings and we will be able to desugar at parse time into Javascript and some runtime calls, but I might be mistaken.
On 2014/10/28 14:25:14, Dmitry Lomov (chromium) wrote:
> Before this CL can land, I think we need a clear design for implementation of
> this feature.
>
> In particular:
> 1. What the code generation would look like, on a high level?
> 2. How will we cache template call sites?
>
> It is my guess that after we figure out 1 and 2, we will realize that we do
not
> actually need new AST nodes for template strings and we will be able to
desugar
> at parse time into Javascript and some runtime calls, but I might be mistaken.
So, there are two codegen paths that matter here, I think:
1) No tag:
1. Assert that cookedStrings.length === expressions.length + 1
2. If 0 expressions: plug the cooked string (TV) -> like string literal,
break.
3. setup scratch variable which contains a JSArray
4. Push cookedStrings(0) to scratch variable
5. For each index 0..N expression:
a. VisitForAccumulatorValue(expression i)
b. Push accumulator value to scratch variable
d. Push cookedStrings(++i) to scratch variable
6. __ Push(scratchVariable)
7. __ CallBuiltin(STRING_CONCAT, CALL_FUNCTION)
8. Plug result of STRING_CONCAT
The STRING_CONCAT builtin will receive a JSArray of cooked strings interleaved
with expression values, and return the result of
`String.prototype.concat.apply('', interleavedElements)`
--- This works nicely, but I can't figure out how to set up a scratch variable
with no binding, so that it doesn't leak into the programme.
2) With tag:
More complicated: Assert that cookedStrings.length == rawStrings.length,
assert that cookedStrings.length == expressions.length + 1
Setup 3 scratch variables, all JSArrays
Push each cookedString into first scratch variable
Push each rawString into second scratchVariable
For each expression, VisitForAccumulatorValue() and push result into third
scratch variable
__ Push(tagFn)
__ Push(scratchCookedStrings)
__ Push(scratchRawStrings)
__ Push(scratchExpressionValues)
__ CallBuiltin(CALL_TAGGED_TEMPLATE, CALL_FUNCTION)
Plug result of that builtin call
The builtin will A) call GetTemplateCallSite() with the first 2 scratch
variables, and B) apply tagFn with [callSite, ...scratchExpressionValues].
The callsite will be attached to the ASTNode so that it need not be re-created
again. (steps 1 and 12 in GetTemplateCallSite())
---
So the challenge (for me) is figuring out how to setup each scratch variable in
a way that doesn't leak into the programme or clobber any
current accumulator value. I haven't figured out how to set up scratch variables
of a specific type, but they really need to be arrays, I think.
So, someone who knows the codegen side more than me could help figure out a way
to make those algorithms a reality, or else help figure out a more efficient
algorithm, if one exists.
I've implemented arv@'s suggestion to generate code for untagged templates as a
tree of BinaryOps --- it works pretty nicely, here's output from d8:
```
d8> function x(i) { print(i); return i; }
undefined
d8> `foo${x(1)}${x(2)}`
1
2
"foo12"
```
So I'll upload the changes and look at writing some test cases for it, but it's
pretty cool. Unfortunately, tagged templates are still under water.
Patchset #3 (id:40001) has been deleted
On 2014/10/29 01:06:56, caitp wrote:
> I've implemented arv@'s suggestion to generate code for untagged templates as
a
> tree of BinaryOps --- it works pretty nicely, here's output from d8:
>
> ```
> d8> function x(i) { print(i); return i; }
> undefined
> d8> `foo${x(1)}${x(2)}`
> 1
> 2
> "foo12"
> ```
>
> So I'll upload the changes and look at writing some test cases for it, but
it's
> pretty cool. Unfortunately, tagged templates are still under water.
Right, the key issue with tagged is where and how to cache the template call
site.
We cannot cache it on AST node since AST nodes are very transient - they only
exist while parsing and compiling. If the function is recompiled (e.g. by the
hydrogen) the AST is rebuilt.
I think the right way to proceed here would be to write a design doc (Google
docs work fine for this), and share it so that we can have a discussion on the
implemnentation there.
I know I already sent an invite to reviewers, but posting it here so others can submit ideas if they like. https://docs.google.com/document/d/1Ss71XkXXSPxP3_kf4L_3vlPCWvSs2dby0aq8Hjah8...
Added rudimentary (slightly incorrect) support for tagged templates. This doesn't include any caching strategy, so a new callSite is created each time the tag is invoked. This isn't ideal, but it's not a bad start maybe.
Patchset #5 (id:100001) has been deleted
https://codereview.chromium.org/663683006/diff/120001/test/mjsunit/es6/templa... File test/mjsunit/es6/templates.js (right): https://codereview.chromium.org/663683006/diff/120001/test/mjsunit/es6/templa... test/mjsunit/es6/templates.js:76: // assertEquals("", s.raw[0]); If I could get some help with this, that would be good. It's not super clear to me why the raw value seems to contain a bunch of old gunk. Will look at it again in a bit
https://codereview.chromium.org/663683006/diff/120001/test/mjsunit/es6/templa... File test/mjsunit/es6/templates.js (right): https://codereview.chromium.org/663683006/diff/120001/test/mjsunit/es6/templa... test/mjsunit/es6/templates.js:76: // assertEquals("", s.raw[0]); On 2014/11/01 03:33:55, caitp wrote: > If I could get some help with this, that would be good. It's not super clear to > me why the raw value seems to contain a bunch of old gunk. Will look at it again > in a bit Nevermind, it was a mistake in the scanner, easily fixed.
Just FWIW, https://www.dropbox.com/s/5f9sq0tqifkmsvc/spidermonkey_does_it_wrong_too.png?... SpiderMonkey is not caching the CallSite either, so maybe it's not a blocker for landing. It would be good to fix this, though.
On 2014/11/01 06:17:58, caitp wrote: > Just FWIW, > https://www.dropbox.com/s/5f9sq0tqifkmsvc/spidermonkey_does_it_wrong_too.png?... > SpiderMonkey is not caching the CallSite either, so maybe it's not a blocker for > landing. It would be good to fix this, though. wait, this test is kind of bogus... so they do actually cache this. I guess it probably is a blocker :<
https://codereview.chromium.org/663683006/diff/160001/src/parser.cc File src/parser.cc (right): https://codereview.chromium.org/663683006/diff/160001/src/parser.cc#newcode5029 src/parser.cc:5029: So when it comes to caching the callSiteObj, something that I've tried to do here is create a new INTERNAL mode variable with scope_->NewInternal(), attach it to a VariableProxy, and pass the VariableProxy as a third argument to GetTemplateCallSite() --- and finally, assign the result of GetTemplateCallSite() to the INTERNAL variable. I've tried a couple of different approaches, but weirdly I can't seem to get a real assignment happening, and the internal variable is always undefined. I think this is the simplest caching mechanism until there's a better way for this to work, but I might need some pointers on getting it to actually "work".
Given that the spec resolution of the caching semantics will probably take a while (I guess next TC39 meeting), I suggest leaving out caching entirely for this CL, and defer it to later. In practice, it shouldn't matter much.
On 2014/11/07 13:03:28, rossberg wrote: > Given that the spec resolution of the caching semantics will probably take a > while (I guess next TC39 meeting), I suggest leaving out caching entirely for > this CL, and defer it to later. In practice, it shouldn't matter much. We could look at trying to match SpiderMonkey behaviour in the meantime, if people think that's valuable
On 2014/11/07 13:45:36, caitp wrote: > On 2014/11/07 13:03:28, rossberg wrote: > > Given that the spec resolution of the caching semantics will probably take a > > while (I guess next TC39 meeting), I suggest leaving out caching entirely for > > this CL, and defer it to later. In practice, it shouldn't matter much. > > We could look at trying to match SpiderMonkey behaviour in the meantime, if > people think that's valuable I don't think it matters for now, let's just do the simplest thing. (The caching is mostly a performance hack in the spec anyway, and one that probably doesn't even help much, due to its administrative overhead.)
On 2014/11/07 13:45:36, caitp wrote: > On 2014/11/07 13:03:28, rossberg wrote: > > Given that the spec resolution of the caching semantics will probably take a > > while (I guess next TC39 meeting), I suggest leaving out caching entirely for > > this CL, and defer it to later. In practice, it shouldn't matter much. > > We could look at trying to match SpiderMonkey behaviour in the meantime, if > people think that's valuable http://jsfiddle.net/caitp/d3ge2u2z/ from this fiddle (in FF Nightly 36.0a1 (2014-11-07)), it looks like `eval` and `new Function()` are never sharing callsites, and in fact `new Function()` results in a ReferenceError.
On 2014/11/07 14:00:47, rossberg wrote: > On 2014/11/07 13:45:36, caitp wrote: > > On 2014/11/07 13:03:28, rossberg wrote: > > > Given that the spec resolution of the caching semantics will probably take a > > > while (I guess next TC39 meeting), I suggest leaving out caching entirely > for > > > this CL, and defer it to later. In practice, it shouldn't matter much. > > > > We could look at trying to match SpiderMonkey behaviour in the meantime, if > > people think that's valuable > > I don't think it matters for now, let's just do the simplest thing. (The caching > is mostly a performance hack in the spec anyway, and one that probably doesn't > even help much, due to its administrative overhead.) Ah, alright. Fair enough
On 2014/11/07 14:00:47, rossberg wrote: > ... (The caching > is mostly a performance hack in the spec anyway, and one that probably doesn't > even help much, due to its administrative overhead.) Just to be clear here. The caching is not a performance hack. It is very important feature that allows you to cache expensive operations.
On 2014/11/07 14:18:36, arv wrote: > On 2014/11/07 14:00:47, rossberg wrote: > > ... (The caching > > is mostly a performance hack in the spec anyway, and one that probably doesn't > > even help much, due to its administrative overhead.) > > Just to be clear here. The caching is not a performance hack. It is very > important feature that allows you to cache expensive operations. Well, the idea of specialising caching by use site is rather bizarre, if you ask me. I would assume that it is perfectly possible to implement any relevant caching without that.
On 2014/11/07 13:03:28, rossberg wrote: > Given that the spec resolution of the caching semantics will probably take a > while (I guess next TC39 meeting), I suggest leaving out caching entirely for > this CL, and defer it to later. In practice, it shouldn't matter much. The q for implementation is, can we desugar in parser completely or shall we have TemplateLiteral AST node. I believe that for non-tagged templates we should desugar in parser in any case. I suggest we implement that, and then figure out what to do with tagged templates. (If you all agree, let's reflect this in design doc and update intent-to-implement)
On 2014/11/07 15:22:59, Dmitry Lomov (chromium) wrote: > On 2014/11/07 13:03:28, rossberg wrote: > > Given that the spec resolution of the caching semantics will probably take a > > while (I guess next TC39 meeting), I suggest leaving out caching entirely for > > this CL, and defer it to later. In practice, it shouldn't matter much. > > The q for implementation is, can we desugar in parser completely or shall we > have TemplateLiteral AST node. > I believe that for non-tagged templates we should desugar in parser in any case. > > I suggest we implement that, and then figure out what to do with tagged > templates. > > (If you all agree, let's reflect this in design doc and update > intent-to-implement) We're already desugaring everything in the parser, so the AST node isn't really needed currently --- for caching, we may need to have a separate AST node (but I think it should still be possible to do this with declared INTERNAL variables, so the extra AST node is probably still not really needed). It should not need any special runtime processing that isn't already possible.
On 2014/11/07 15:30:02, caitp wrote: > On 2014/11/07 15:22:59, Dmitry Lomov (chromium) wrote: > > On 2014/11/07 13:03:28, rossberg wrote: > > > Given that the spec resolution of the caching semantics will probably take a > > > while (I guess next TC39 meeting), I suggest leaving out caching entirely > for > > > this CL, and defer it to later. In practice, it shouldn't matter much. > > > > The q for implementation is, can we desugar in parser completely or shall we > > have TemplateLiteral AST node. > > I believe that for non-tagged templates we should desugar in parser in any > case. > > > > I suggest we implement that, and then figure out what to do with tagged > > templates. > > > > (If you all agree, let's reflect this in design doc and update > > intent-to-implement) > > We're already desugaring everything in the parser, so the AST node isn't really > needed currently --- for caching, we may need to have a separate AST node (but I > think it should still be possible to do this with declared INTERNAL variables, > so the extra AST node is probably still not really needed). It should not need > any special runtime processing that isn't already possible. I don't think INTERNAL variables will work, for the reasons stated earlier (the state is more global than that, I assume it has to be per native context, essentially). The easiest strategy would be to desugar into a call to a builtin or runtime function to query the global cache.
On 2014/11/07 15:39:17, rossberg wrote: > On 2014/11/07 15:30:02, caitp wrote: > > On 2014/11/07 15:22:59, Dmitry Lomov (chromium) wrote: > > > On 2014/11/07 13:03:28, rossberg wrote: > > > > Given that the spec resolution of the caching semantics will probably take > a > > > > while (I guess next TC39 meeting), I suggest leaving out caching entirely > > for > > > > this CL, and defer it to later. In practice, it shouldn't matter much. > > > > > > The q for implementation is, can we desugar in parser completely or shall we > > > have TemplateLiteral AST node. > > > I believe that for non-tagged templates we should desugar in parser in any > > case. > > > > > > I suggest we implement that, and then figure out what to do with tagged > > > templates. > > > > > > (If you all agree, let's reflect this in design doc and update > > > intent-to-implement) > > > > We're already desugaring everything in the parser, so the AST node isn't > really > > needed currently --- for caching, we may need to have a separate AST node (but > I > > think it should still be possible to do this with declared INTERNAL variables, > > so the extra AST node is probably still not really needed). It should not need > > any special runtime processing that isn't already possible. Ah, I see, missed that you've updated the patch! I'll try to get to reviewing this patch soon-ish (unless other folks get to it first :)). > > I don't think INTERNAL variables will work, for the reasons stated earlier (the > state is more global than that, I assume it has to be per native context, > essentially). The easiest strategy would be to desugar into a call to a builtin > or runtime function to query the global cache. +1
arv@chromium.org changed reviewers: + marja@chromium.org
Adding marja since she is the parser expert. One thing that I was thinking is that maybe we could skip the flag in the scanner. If we need the raw value we can get that straight from the source code since we know the location of the TemplateHead/TemplateSpan/TemplateTail. https://codereview.chromium.org/663683006/diff/180001/src/harmony-templates.js File src/harmony-templates.js (right): https://codereview.chromium.org/663683006/diff/180001/src/harmony-templates.j... src/harmony-templates.js:7: function GetTemplateCallSite(cookedStrings, rawStrings) { Eventually I think we need to pass in some kind of ID (a hash that is computed at parse time). If we did that we could keep a Map in this file with the cache. https://codereview.chromium.org/663683006/diff/180001/test/cctest/test-parsin... File test/cctest/test-parsing.cc (right): https://codereview.chromium.org/663683006/diff/180001/test/cctest/test-parsin... test/cctest/test-parsing.cc:4321: "`no-subst-template`", Also include an empty one `` https://codereview.chromium.org/663683006/diff/180001/test/cctest/test-parsin... test/cctest/test-parsing.cc:4323: "`${a}template-tail`", How about one with only a place holder `${a}` https://codereview.chromium.org/663683006/diff/180001/test/mjsunit/es6/templa... File test/mjsunit/es6/templates.js (right): https://codereview.chromium.org/663683006/diff/180001/test/mjsunit/es6/templa... test/mjsunit/es6/templates.js:28: // Multi-line templates How about testing one with \r\n too?
I agree it would be nice to get the raw text in a better way, but there are so many different rules for TRV that I wasn't sure if it would be safe to do it that way. At least for line endings they need to be normalized, and I'm not sure the scanner does this on its own. https://codereview.chromium.org/663683006/diff/180001/test/cctest/test-parsin... File test/cctest/test-parsing.cc (right): https://codereview.chromium.org/663683006/diff/180001/test/cctest/test-parsin... test/cctest/test-parsing.cc:4321: "`no-subst-template`", On 2014/11/07 16:55:50, arv wrote: > Also include an empty one > > `` Done. https://codereview.chromium.org/663683006/diff/180001/test/cctest/test-parsin... test/cctest/test-parsing.cc:4323: "`${a}template-tail`", On 2014/11/07 16:55:50, arv wrote: > How about one with only a place holder > > `${a}` Done. https://codereview.chromium.org/663683006/diff/180001/test/mjsunit/es6/templa... File test/mjsunit/es6/templates.js (right): https://codereview.chromium.org/663683006/diff/180001/test/mjsunit/es6/templa... test/mjsunit/es6/templates.js:28: // Multi-line templates On 2014/11/07 16:55:50, arv wrote: > How about testing one with \r\n too? the trouble with that (from an mjsunit test) is that it's likely the line endings in the test file would be normalized to plain LFs at some point. It would make a good API test because then there's some control over the line endings.
https://codereview.chromium.org/663683006/diff/180001/test/mjsunit/es6/templa... File test/mjsunit/es6/templates.js (right): https://codereview.chromium.org/663683006/diff/180001/test/mjsunit/es6/templa... test/mjsunit/es6/templates.js:28: // Multi-line templates On 2014/11/07 17:13:08, caitp wrote: > On 2014/11/07 16:55:50, arv wrote: > > How about testing one with \r\n too? > > the trouble with that (from an mjsunit test) is that it's likely the line > endings in the test file would be normalized to plain LFs at some point. > > It would make a good API test because then there's some control over the line > endings. You should be able to use eval to be explicit about the line endings.
https://codereview.chromium.org/663683006/diff/180001/test/mjsunit/es6/templa... File test/mjsunit/es6/templates.js (right): https://codereview.chromium.org/663683006/diff/180001/test/mjsunit/es6/templa... test/mjsunit/es6/templates.js:28: // Multi-line templates On 2014/11/07 17:14:18, arv wrote: > You should be able to use eval to be explicit about the line endings. Ah, you're right, I guess I'm not thinking very creatively about this :)
I started having a look, but then at some point my confusion exceeded a threshold. Maybe you can use it as guidance on where to add comments / clarifications. :) https://codereview.chromium.org/663683006/diff/220001/src/preparser.h File src/preparser.h (right): https://codereview.chromium.org/663683006/diff/220001/src/preparser.h#newcode... src/preparser.h:2506: result->AsFunctionLiteral()->set_parenthesized(); Why? https://codereview.chromium.org/663683006/diff/220001/src/preparser.h#newcode... src/preparser.h:2851: ParserBase<Traits>::ParseTemplateLiteral(ExpressionT tag, int start, bool* ok) { Can you add here a comment about the parse rules which describe the syntax (like the comments many other parsing functions have)? https://codereview.chromium.org/663683006/diff/220001/src/preparser.h#newcode... src/preparser.h:2870: ExpressionT expression = this->ParseExpression(false, CHECK_OK); Why expression? Are we now inside the ${... or... what? No, not necessarily? *Confused* https://codereview.chromium.org/663683006/diff/220001/src/scanner.cc File src/scanner.cc (right): https://codereview.chromium.org/663683006/diff/220001/src/scanner.cc#newcode797 src/scanner.cc:797: Token::Value Scanner::ScanTemplateSpan() { Hmm, I don't fully understand this function. Pls add comments: 1) about how literal_chars_ and raw_literal_chars_ are used and why (I was briefly confused when I read that part at first...) 2) about which part exactly this function scans; what's the returned token and what's in the literal buffer... I'm also confused because the terminology doesn't seem to match the spec draft... what's "TEMPLATE_SPAN"? (The spec has stuff like template head, template middle, template tail..) https://codereview.chromium.org/663683006/diff/220001/test/mjsunit/es6/templa... File test/mjsunit/es6/templates.js (right): https://codereview.chromium.org/663683006/diff/220001/test/mjsunit/es6/templa... test/mjsunit/es6/templates.js:170: (function(s) { assertEquals("\u005C\u0030", s.raw[0]); })`\0`; I was confused about why you write \u005C here, at this point it's inside a normal string so it should be the same as \\... I guess this is why: "The TRV of TemplateCharacter :: \ EscapeSequence is the sequence consisting of the code unit value 0x005C followed by the code units of TRV of EscapeSequence."
Thanks for the look --- I will add some comments to explain things a bit, but I hope the replies help make sense of it a bit! https://codereview.chromium.org/663683006/diff/220001/src/preparser.h File src/preparser.h (right): https://codereview.chromium.org/663683006/diff/220001/src/preparser.h#newcode... src/preparser.h:2506: result->AsFunctionLiteral()->set_parenthesized(); On 2014/11/10 15:32:02, marja wrote: > Why? It's basically copy/pasted from the MemberExpression -> CallExpression path --- so, presumably if we have an IIFE we need to do this, but I'm not sure what difference it makes. https://codereview.chromium.org/663683006/diff/220001/src/preparser.h#newcode... src/preparser.h:2870: ExpressionT expression = this->ParseExpression(false, CHECK_OK); On 2014/11/10 15:32:02, marja wrote: > Why expression? Are we now inside the ${... or... what? No, not necessarily? > *Confused* Yeah, if we get a TEMPLATE_SPAN token, then we just finished parsing `${`, and are ready to parse an expression. I'll add a comment about this. Do you think we could combine TEMPLATE_SPAN and TEMPLATE_TAIL into a single token somehow? I'm not sure how that would work properly https://codereview.chromium.org/663683006/diff/220001/src/scanner.cc File src/scanner.cc (right): https://codereview.chromium.org/663683006/diff/220001/src/scanner.cc#newcode797 src/scanner.cc:797: Token::Value Scanner::ScanTemplateSpan() { On 2014/11/10 15:32:02, marja wrote: > Hmm, I don't fully understand this function. > > Pls add comments: > > 1) about how literal_chars_ and raw_literal_chars_ are used and why (I was > briefly confused when I read that part at first...) > 2) about which part exactly this function scans; what's the returned token and > what's in the literal buffer... > > I'm also confused because the terminology doesn't seem to match the spec > draft... what's "TEMPLATE_SPAN"? (The spec has stuff like template head, > template middle, template tail..) literal_chars_ are being used as the TV or cooked value of the template (escaped characters -> unescaped form, basically), raw_literal_chars_ are being used as the TRV or raw value of the template (escaped characters as they appear in the source, basically). arv@ has suggested that we do this without keeping a separate string for the raw values, but I don't know how it will actually work in all cases, since raw values need to normalize line endings anyways. Ideas on that are welcome. So the spec has concept of template head -> template middle -> template end -> etc, but in practice I think there are only 2 cases that matter: the case where you have a literal span followed by an expression, and the case where you have just a literal span. TEMPLATE_SPAN -> always followed by an expression (ends with ${), while TEMPLATE_TAIL -> always followed by ` or the end of the template literal (if not, then it's a bad token). Basically, all of the NoSubstTemplate productions become TEMPLATE_TAIL, and all of the other template productions become TEMPLATE_SPAN, I think it's simpler this way https://codereview.chromium.org/663683006/diff/220001/test/mjsunit/es6/templa... File test/mjsunit/es6/templates.js (right): https://codereview.chromium.org/663683006/diff/220001/test/mjsunit/es6/templa... test/mjsunit/es6/templates.js:170: (function(s) { assertEquals("\u005C\u0030", s.raw[0]); })`\0`; On 2014/11/10 15:32:02, marja wrote: > I was confused about why you write \u005C here, at this point it's inside a > normal string so it should be the same as \\... > > I guess this is why: > > "The TRV of TemplateCharacter :: \ EscapeSequence is the sequence consisting of > the code unit value 0x005C followed by the code units of TRV of EscapeSequence." Yeah --- plus I found it a bit harder to read with single-escape-characters. I'll change it if people prefer that though =)
Added some comments to explain the parsing and scanning of template literals, I hope that makes it easier =)
Thx; it's much clearer now. I didn't yet have a close look at CloseTemplateLiteral, will do that tomorrow. It would be theoretically possible to just store a representation of the raw literal, since we can always get the cooked literal from it (we'd need to store \uxxx instead of uxxx like we do now), and then we could have a conversion function... but idk if that would make the code any cleaner or just messier. https://codereview.chromium.org/663683006/diff/240001/src/preparser.h File src/preparser.h (right): https://codereview.chromium.org/663683006/diff/240001/src/preparser.h#newcode... src/preparser.h:2918: return Traits::CloseTemplateLiteral(&ts, start, tag); Nit: there's only one break above, so you can move this line there, and DCHECK(false); here (the only way to get away from the infinite loop is to return, right, we don't want to do any actions after it if I understood it right).
caitp, wdyt about arv's idea: not storing the raw string but getting it from the source code if needed? That would simplify the parsing / scanning part. Would that be feasible?
To continue our discussion in the old patch set... (To maximize confusion, I'll post new comments in the context of the latest patch set.) https://codereview.chromium.org/663683006/diff/220001/src/preparser.h File src/preparser.h (right): https://codereview.chromium.org/663683006/diff/220001/src/preparser.h#newcode... src/preparser.h:2506: result->AsFunctionLiteral()->set_parenthesized(); On 2014/11/10 15:43:56, caitp wrote: > On 2014/11/10 15:32:02, marja wrote: > > Why? > > It's basically copy/pasted from the MemberExpression -> CallExpression path --- > so, presumably if we have an IIFE we need to do this, but I'm not sure what > difference it makes. I dug up a bit where this is coming from. In any case, it needs a comment. So, the original code is in ParseLeftHandSideExpression, and afaics, it's meant to recognize this pattern: function foo() { ... } ( ^ result is a FunctionLiteral ^ just saw the LPAREN so in that case, that's a hint that the function will be called right away, and set_parenthesized is called to force eager compilation. Fair enough. So, here, the corresponding situation would be e.g., function foo() { ... } `tail` ^ result is a FunctionLiteral ^ just saw the TEMPLATE_TAIL And this should be explained in the comment.
Alright, here are some more comments. These are pretty much trivial. The biggest question mark at this point is the raw literal value collection vs. reading it from the source code. Pls also see my other comment in the previous patch set. In general this looks pretty good, I'm generally convinced about this CL. And the test is good! https://codereview.chromium.org/663683006/diff/260001/src/ast-value-factory.h File src/ast-value-factory.h (right): https://codereview.chromium.org/663683006/diff/260001/src/ast-value-factory.h... src/ast-value-factory.h:251: F(GetTemplateCallSite, "GetTemplateCallSite") \ You'd probably want to change the first part to get_template_call_site, for consistency (see below make_reference_error etc.). https://codereview.chromium.org/663683006/diff/260001/src/preparser.h File src/preparser.h (right): https://codereview.chromium.org/663683006/diff/260001/src/preparser.h#newcode... src/preparser.h:1400: void AddTemplateSpan(TemplateLiteralState* state) { USE(state); } Shorter (here and below): void AddTemplateSpan(TemplateLiteralState*) {} (if you omit the parameter name, you don't need USE) https://codereview.chromium.org/663683006/diff/260001/src/preparser.h#newcode... src/preparser.h:2877: // Add TV / TRV This comment is confusing... which part is adding the TV / TRV? https://codereview.chromium.org/663683006/diff/260001/src/preparser.h#newcode... src/preparser.h:2887: for (;;) { Style nit: while(true) seems to be more common. https://codereview.chromium.org/663683006/diff/260001/src/preparser.h#newcode... src/preparser.h:2895: // Parse an Expression Nit: this comment is not needed; it's clear that it parses an expression. The comments should explain the intention (like the bigger comment in the beginning of ParseTemplateLiteral does) and the should explain why the code is written exactly the way it is but only if it's nontrivial ("a must be done before b, because..."). https://codereview.chromium.org/663683006/diff/260001/src/scanner.cc File src/scanner.cc (right): https://codereview.chromium.org/663683006/diff/260001/src/scanner.cc#newcode819 src/scanner.cc:819: PushBack('}'); Hmm, when does this happen and why do we PushBack?
On 2014/11/11 08:51:03, marja wrote: > caitp, wdyt about arv's idea: not storing the raw string but getting it from the > source code if needed? That would simplify the parsing / scanning part. Would > that be feasible? It should be feasible, but I've not yet seen how I can do that, and I expect it would still be messy due to the line ending normalization that needs to happen. I'll look at doing that today though https://codereview.chromium.org/663683006/diff/260001/src/preparser.h File src/preparser.h (right): https://codereview.chromium.org/663683006/diff/260001/src/preparser.h#newcode... src/preparser.h:2877: // Add TV / TRV On 2014/11/11 09:47:24, marja wrote: > This comment is confusing... which part is adding the TV / TRV? I think that's a stray comment, doesn't look like it belongs here https://codereview.chromium.org/663683006/diff/260001/src/scanner.cc File src/scanner.cc (right): https://codereview.chromium.org/663683006/diff/260001/src/scanner.cc#newcode819 src/scanner.cc:819: PushBack('}'); On 2014/11/11 09:47:24, marja wrote: > Hmm, when does this happen and why do we PushBack? After parsing an expression, the scanner ends up with a peeked RBRACE and we're at the wrong position in source, I'm not sure if this is the right way to deal with that but it seems to work. Without this, it if you have a template like `${a}`, it misses the ending backtick and breaks, which is bad
https://codereview.chromium.org/663683006/diff/260001/src/scanner.cc File src/scanner.cc (right): https://codereview.chromium.org/663683006/diff/260001/src/scanner.cc#newcode819 src/scanner.cc:819: PushBack('}'); On 2014/11/11 13:59:29, caitp wrote: > On 2014/11/11 09:47:24, marja wrote: > > Hmm, when does this happen and why do we PushBack? > > After parsing an expression, the scanner ends up with a peeked RBRACE and we're > at the wrong position in source, I'm not sure if this is the right way to deal > with that but it seems to work. > > Without this, it if you have a template like `${a}`, it misses the ending > backtick and breaks, which is bad But after this, we anyway do Advance() right away. Would the code be clearer if it was restructured to be PushBack-free?
On 2014/11/11 15:01:56, marja wrote: > https://codereview.chromium.org/663683006/diff/260001/src/scanner.cc > File src/scanner.cc (right): > > https://codereview.chromium.org/663683006/diff/260001/src/scanner.cc#newcode819 > src/scanner.cc:819: PushBack('}'); > On 2014/11/11 13:59:29, caitp wrote: > > On 2014/11/11 09:47:24, marja wrote: > > > Hmm, when does this happen and why do we PushBack? > > > > After parsing an expression, the scanner ends up with a peeked RBRACE and > we're > > at the wrong position in source, I'm not sure if this is the right way to deal > > with that but it seems to work. > > > > Without this, it if you have a template like `${a}`, it misses the ending > > backtick and breaks, which is bad > > But after this, we anyway do Advance() right away. Would the code be clearer if > it was restructured to be PushBack-free? It looks simpler to me to just PushBack() --- as it's used, it looks like it should be safe to do it this way, and it means it's safe to make assertions about the conditions the function is called with, just my opinion though :) I've changed the code to pull raw strings out of Script::source(), but it's a bit more complicated this way, and I'm not positive if this would work correctly (or efficiently) in all cases.
Alright, I'll have a look tomorrow since the day is over on this time zone. I think I had misunderstood the pushback part, your comment makes it clearer :)
Very nice. https://codereview.chromium.org/663683006/diff/320001/src/harmony-templates.js File src/harmony-templates.js (right): https://codereview.chromium.org/663683006/diff/320001/src/harmony-templates.j... src/harmony-templates.js:14: for (; index < count; ++index) { why not? for (var index = 0; index < count; ++index) { https://codereview.chromium.org/663683006/diff/320001/src/parser.cc File src/parser.cc (right): https://codereview.chromium.org/663683006/diff/320001/src/parser.cc#newcode5097 src/parser.cc:5097: ALLOW_NULLS, FAST_STRING_TRAVERSAL, offset+1, length-1, &length); nit: ws around + and - https://codereview.chromium.org/663683006/diff/320001/src/parser.cc#newcode5130 src/parser.cc:5130: ast_value_factory()->get_template_callsite_string(), NULL, args, start); I'm not sure but isn't it better to use the function id here? https://codereview.chromium.org/663683006/diff/320001/src/parser.cc#newcode5138 src/parser.cc:5138: if (fni_ != NULL) fni_->RemoveLastFunction(); I don't see where the function was added? https://codereview.chromium.org/663683006/diff/320001/src/parser.h File src/parser.h (right): https://codereview.chromium.org/663683006/diff/320001/src/parser.h#newcode976 src/parser.h:976: ParserTraits::TemplateLiteralState ParserTraits::OpenTemplateLiteral(int pos) { nit: 2 empty lines between functions https://codereview.chromium.org/663683006/diff/320001/src/preparser.h File src/preparser.h (right): https://codereview.chromium.org/663683006/diff/320001/src/preparser.h#newcode... src/preparser.h:2910: DCHECK(false); UNREACHABLE(); But maybe change it to: DCHECK_EQ(next, Token::TEMPLATE_TAIL); // Once we've reached a TEMPLATE_TAIL, we can close the TemplateLiteral. return Traits::CloseTemplateLiteral(&ts, start, tag); https://codereview.chromium.org/663683006/diff/320001/src/scanner.cc File src/scanner.cc (right): https://codereview.chromium.org/663683006/diff/320001/src/scanner.cc#newcode683 src/scanner.cc:683: bool singleCharEscape = true; This is no longer used https://codereview.chromium.org/663683006/diff/320001/test/mjsunit/es6/templa... File test/mjsunit/es6/templates.js (right): https://codereview.chromium.org/663683006/diff/320001/test/mjsunit/es6/templa... test/mjsunit/es6/templates.js:7: var num = 5; I prefer writing these tests in functions. Makes it easier to read. (function TestBasicExpressions() { ... })(); (function TestExpressionsContainingTemplates() { ... })(); (function TestTaggedTemplates() { ... })(); ...
https://codereview.chromium.org/663683006/diff/320001/src/parser.cc File src/parser.cc (right): https://codereview.chromium.org/663683006/diff/320001/src/parser.cc#newcode5138 src/parser.cc:5138: if (fni_ != NULL) fni_->RemoveLastFunction(); On 2014/11/11 17:15:15, arv wrote: > I don't see where the function was added? it's copy-pasted, I tried to make sense of it based on the code for producing CallExpressions --- if it doesn't make sense here, it will disappear :)
Patchset #17 (id:360001) has been deleted
Patchset #17 (id:380001) has been deleted
LG modulo these comments. There's one bug (missing setting the flag), and the rest is variable naming and style nits. In general, the code looks very nice like arv@ said above, and IMO the latest patch set is a clear improvement over the previous versions. Thanks for working on this! https://codereview.chromium.org/663683006/diff/400001/src/parser.cc File src/parser.cc (right): https://codereview.chromium.org/663683006/diff/400001/src/parser.cc#newcode3914 src/parser.cc:3914: allow_harmony_object_literals()); Here you need to set the allow_templates flag to reusable_preparser as well. https://codereview.chromium.org/663683006/diff/400001/src/parser.cc#newcode5062 src/parser.cc:5062: const ZoneList<Expression*>* cookedStrings = lit->cooked(); Style: cookedStrings -> cooked_strings (+ other variables elsewhere) https://codereview.chromium.org/663683006/diff/400001/src/parser.cc#newcode5070 src/parser.cc:5070: if (!expressions->length()) { Style nit: if (expressions->length() == 0) https://codereview.chromium.org/663683006/diff/400001/src/parser.cc#newcode5088 src/parser.cc:5088: ZoneList<Expression*>* rawStrings = new (zone()) ZoneList<Expression*>( raw_strings https://codereview.chromium.org/663683006/diff/400001/src/parser.cc#newcode5093 src/parser.cc:5093: for (int i = 0; i < cookedStrings->length(); ++i) { Gets confusing with i, c and j; could you rename i to string_index or sth, then j could be raw_chars_from_index and c could be raw_chars_to_index. Or something along those lines. https://codereview.chromium.org/663683006/diff/400001/src/parser.cc#newcode5101 src/parser.cc:5101: // Normalize line endings You could expand this comment to say what exactly is changed to what. It's not trivial to see from the code below.... \r\n is changed to \n, right? https://codereview.chromium.org/663683006/diff/400001/src/parser.cc#newcode5117 src/parser.cc:5117: I like this version of raw string building thing better than the previous version, since this is localized: this quirk of templates doesn't leak to all places where we handle literals. https://codereview.chromium.org/663683006/diff/400001/src/parser.cc#newcode5133 src/parser.cc:5133: ZoneList<Expression*>* callArgs = new (zone()) ZoneList<Expression*>( call_args https://codereview.chromium.org/663683006/diff/400001/src/scanner.cc File src/scanner.cc (right): https://codereview.chromium.org/663683006/diff/400001/src/scanner.cc#newcode836 src/scanner.cc:836: // consisting of the CV 0x000A. I know this is directly from the spec, but... wut? What's the difference between "the CV 0x000A" and "the sequence consisting of the CV 0x000A"?
Oh and pls change the description of the CL to something more descriptive as a preparation step for landing this soon!
Can somebody (arv@?) have a look at src/harmony-templates.js, I can't say whether it's correct or not.
Addressed some nits... Last thing is probably to move generation of "raw" strings into own function, for readability, as it is quite difficult to understand now. https://codereview.chromium.org/663683006/diff/400001/src/parser.cc File src/parser.cc (right): https://codereview.chromium.org/663683006/diff/400001/src/parser.cc#newcode3914 src/parser.cc:3914: allow_harmony_object_literals()); On 2014/11/12 09:23:21, marja wrote: > Here you need to set the allow_templates flag to reusable_preparser as well. Done. https://codereview.chromium.org/663683006/diff/400001/src/parser.cc#newcode5062 src/parser.cc:5062: const ZoneList<Expression*>* cookedStrings = lit->cooked(); On 2014/11/12 09:23:21, marja wrote: > Style: cookedStrings -> cooked_strings (+ other variables elsewhere) Done. https://codereview.chromium.org/663683006/diff/400001/src/parser.cc#newcode5070 src/parser.cc:5070: if (!expressions->length()) { On 2014/11/12 09:23:21, marja wrote: > Style nit: if (expressions->length() == 0) Done. https://codereview.chromium.org/663683006/diff/400001/src/parser.cc#newcode5093 src/parser.cc:5093: for (int i = 0; i < cookedStrings->length(); ++i) { On 2014/11/12 09:23:21, marja wrote: > Gets confusing with i, c and j; could you rename i to string_index or sth, then > j could be raw_chars_from_index and c could be raw_chars_to_index. Or something > along those lines. Done. (I'm not sure this is much easier to read, though) https://codereview.chromium.org/663683006/diff/400001/src/parser.cc#newcode5101 src/parser.cc:5101: // Normalize line endings On 2014/11/12 09:23:21, marja wrote: > You could expand this comment to say what exactly is changed to what. It's not > trivial to see from the code below.... \r\n is changed to \n, right? Done.
LGTM modulo the uninitialized variable below. pls wait for review for src/harmony-templates.js before committing. https://codereview.chromium.org/663683006/diff/420001/src/scanner.h File src/scanner.h (right): https://codereview.chromium.org/663683006/diff/420001/src/scanner.h#newcode690 src/scanner.h:690: bool harmony_templates_; This is not initialized!
https://codereview.chromium.org/663683006/diff/400001/src/parser.cc File src/parser.cc (right): https://codereview.chromium.org/663683006/diff/400001/src/parser.cc#newcode5093 src/parser.cc:5093: for (int i = 0; i < cookedStrings->length(); ++i) { On 2014/11/12 13:33:12, caitp wrote: > On 2014/11/12 09:23:21, marja wrote: > > Gets confusing with i, c and j; could you rename i to string_index or sth, > then > > j could be raw_chars_from_index and c could be raw_chars_to_index. Or > something > > along those lines. > > Done. (I'm not sure this is much easier to read, though) Yeah, they're kinda long. How about just to_index and from_index? Or to and from?
On 2014/11/12 13:41:14, marja wrote: > https://codereview.chromium.org/663683006/diff/400001/src/parser.cc > File src/parser.cc (right): > > https://codereview.chromium.org/663683006/diff/400001/src/parser.cc#newcode5093 > src/parser.cc:5093: for (int i = 0; i < cookedStrings->length(); ++i) { > On 2014/11/12 13:33:12, caitp wrote: > > On 2014/11/12 09:23:21, marja wrote: > > > Gets confusing with i, c and j; could you rename i to string_index or sth, > > then > > > j could be raw_chars_from_index and c could be raw_chars_to_index. Or > > something > > > along those lines. > > > > Done. (I'm not sure this is much easier to read, though) > > Yeah, they're kinda long. How about just to_index and from_index? Or to and > from? Ah, that's actually exactly the identifiers I used, what a coincidence. I've moved it into a separate function and made it a bit easier to read, I think it's clearer now.
https://codereview.chromium.org/663683006/diff/440001/src/harmony-templates.js File src/harmony-templates.js (right): https://codereview.chromium.org/663683006/diff/440001/src/harmony-templates.j... src/harmony-templates.js:16: DefineArrayProperty(siteObj, prop, ToPropertyDescriptor({ I feel like this is doing too much work. Nothing in this spec algorithm is observable since we are creating new objects from internal structures. So given that, we should be able to do something simpler: function GetTemplateCallSite(siteObj, rawStrings) { %AddNamedProperty(siteObj, "raw", %ObjectFreeze(rawStrings), READ_ONLY | DONT_ENUM | DONT_DELETE); return %ObjectFreeze(siteObj); } (Even if we wanted to create new arrays we should have added the elements using AddElement. No need to go through descriptors etc.)
https://codereview.chromium.org/663683006/diff/440001/src/harmony-templates.js File src/harmony-templates.js (right): https://codereview.chromium.org/663683006/diff/440001/src/harmony-templates.j... src/harmony-templates.js:16: DefineArrayProperty(siteObj, prop, ToPropertyDescriptor({ On 2014/11/12 15:06:05, arv wrote: > I feel like this is doing too much work. > > Nothing in this spec algorithm is observable since we are creating new objects > from internal structures. So given that, we should be able to do something > simpler: > > function GetTemplateCallSite(siteObj, rawStrings) { > %AddNamedProperty(siteObj, "raw", %ObjectFreeze(rawStrings), > READ_ONLY | DONT_ENUM | DONT_DELETE); > return %ObjectFreeze(siteObj); > } > > (Even if we wanted to create new arrays we should have added the elements using > AddElement. No need to go through descriptors etc.) Done. It's a lot tinier that way.
A few more tests would be useful to prevent regressions. https://codereview.chromium.org/663683006/diff/500001/test/mjsunit/es6/templa... File test/mjsunit/es6/templates.js (right): https://codereview.chromium.org/663683006/diff/500001/test/mjsunit/es6/templa... test/mjsunit/es6/templates.js:38: (function testLineCondition() { testLineCondition -> testLineContinuation https://codereview.chromium.org/663683006/diff/500001/test/mjsunit/es6/templa... test/mjsunit/es6/templates.js:45: var called = false; Tip: Use a number instead and increment it. Then you also catch wrong number of calls. var calls = 0; (function(s) { calls++; })`test`; assertEquals(1, calls); https://codereview.chromium.org/663683006/diff/500001/test/mjsunit/es6/templa... test/mjsunit/es6/templates.js:219: })(); A few more test would not hurt: 1. I know your code behaves correct here but we should add a few more tests for strange Array.prototype. For example using setters. Object.defineProperty(Array.prototype, 0, { set: function() { assertUnreachable(); } }); fn`a${1}b` 2.Can you add tests that the call site object has the correct shape and is froozen? 3. I don't see a test for \` 4. Maybe a test with a BOM is needed too?
https://codereview.chromium.org/663683006/diff/500001/test/mjsunit/es6/templa... File test/mjsunit/es6/templates.js (right): https://codereview.chromium.org/663683006/diff/500001/test/mjsunit/es6/templa... test/mjsunit/es6/templates.js:38: (function testLineCondition() { On 2014/11/12 15:43:15, arv wrote: > testLineCondition -> testLineContinuation Whoops. Done. https://codereview.chromium.org/663683006/diff/500001/test/mjsunit/es6/templa... test/mjsunit/es6/templates.js:219: })(); On 2014/11/12 15:43:15, arv wrote: > A few more test would not hurt: > > 1. > > I know your code behaves correct here but we should add a few more tests for > strange Array.prototype. For example using setters. > > Object.defineProperty(Array.prototype, 0, { > set: function() { > assertUnreachable(); > } > }); > > fn`a${1}b` > > > 2.Can you add tests that the call site object has the correct shape and is > froozen? > > 3. I don't see a test for \` > > 4. Maybe a test with a BOM is needed too? Done. I'm not sure the BOM test makes a lot of sense, but I've added one just to see if it's consistent with FF's behaviour, which it is. I guess a bit more could be done to test the shape of the callSiteObject, but it seems about right.
LGTM We keep the tests for unreleased feature in test/mjsunit/harmony/ and not in test/mjsunit/es6/. Once we remove the flag we also move the tests. https://codereview.chromium.org/663683006/diff/520001/test/mjsunit/es6/templa... File test/mjsunit/es6/templates.js (right): https://codereview.chromium.org/663683006/diff/520001/test/mjsunit/es6/templa... test/mjsunit/es6/templates.js:134: (function(s) { calls++; assertEquals("foo", s[0]); })`foo`; I didn't imply that you need to add this to all your functions. I only suggested that you replace the boolean one above. https://codereview.chromium.org/663683006/diff/520001/test/mjsunit/es6/templa... test/mjsunit/es6/templates.js:290: assertTrue("raw" in cs); Generally, prefer hasOwnProperty or getOwnPropertyDescription since in can imply that the property is available in the prototype chain. https://codereview.chromium.org/663683006/diff/520001/test/mjsunit/es6/templa... test/mjsunit/es6/templates.js:293: cs[1] = true; A better way to test for read only is to assert that the property descriptor has writable == false.
https://codereview.chromium.org/663683006/diff/520001/test/mjsunit/es6/templa... File test/mjsunit/es6/templates.js (right): https://codereview.chromium.org/663683006/diff/520001/test/mjsunit/es6/templa... test/mjsunit/es6/templates.js:134: (function(s) { calls++; assertEquals("foo", s[0]); })`foo`; On 2014/11/12 16:19:10, arv wrote: > I didn't imply that you need to add this to all your functions. I only suggested > that you replace the boolean one above. Yeah, but I think it doesn't hurt for the other cases, since there are slight differences in the parsed pattern, it's good to be thorough. It's a bit redundant, but that's great in tests, so long as it's not problematic https://codereview.chromium.org/663683006/diff/520001/test/mjsunit/es6/templa... test/mjsunit/es6/templates.js:290: assertTrue("raw" in cs); On 2014/11/12 16:19:10, arv wrote: > Generally, prefer hasOwnProperty or getOwnPropertyDescription since in can imply > that the property is available in the prototype chain. Acknowledged. https://codereview.chromium.org/663683006/diff/520001/test/mjsunit/es6/templa... test/mjsunit/es6/templates.js:293: cs[1] = true; On 2014/11/12 16:19:10, arv wrote: > A better way to test for read only is to assert that the property descriptor has > writable == false. Changed it to assert values of property descriptor properties instead
LGTM https://codereview.chromium.org/663683006/diff/540001/test/mjsunit/harmony/te... File test/mjsunit/harmony/templates.js (right): https://codereview.chromium.org/663683006/diff/540001/test/mjsunit/harmony/te... test/mjsunit/harmony/templates.js:297: assertTrue(cs.raw instanceof Array); Similar issue with instanceof as in. In test you are better of being more precise: assertEquals(Array.prototype. Object.getPrototypeOf(cs.raw)); assertTrue(Array.isArray(cs.raw)); You do not really have to change this but next time you write tests keep it in mind.
caitp, should we land this now?
On 2014/11/13 10:48:20, marja wrote: > caitp, should we land this now? I made some last minute adjustments to the way they're scanned (in order to improve error reporting), but tests are still passing and I haven't been able to get it to break doing manual fuzzing, but there is an automated fuzz testing process here right? Other than that, I don't see why not.
On 2014/11/13 14:57:05, caitp wrote: > On 2014/11/13 10:48:20, marja wrote: > > caitp, should we land this now? > > I made some last minute adjustments to the way they're scanned (in order to > improve error reporting), but tests are still passing and I haven't been able to > get it to break doing manual fuzzing, but there is an automated fuzz testing > process here right? > > Other than that, I don't see why not. Maybe somebody on a better time zone can land it today? (I'm leaving for today.) I think our fuzzer needs to start fuzzing with the flag too, but that can be done after landing this, methinks. I'll ping the fuzzer people about it after this has landed. Btw, I would've wanted to have a look at your latest edits, but the latest patch sets seem to be a rebase plus your edits, which makes the "delta between patch sets" feature not useful for this. :( In the future, pls upload rebases as separate patch sets (marked as "rebased" or such) so this is easier.
On 2014/11/13 15:04:03, marja wrote: > Btw, I would've wanted to have a look at your latest edits, but the latest patch > sets seem to be a rebase plus your edits, which makes the "delta between patch > sets" feature not useful for this. :( In the future, pls upload rebases as > separate patch sets (marked as "rebased" or such) so this is easier. Whoops. The changes are very minor: ScanTemplateSpan will never return Token::IlLEGAL, but either Token::EOS or one of the two TEMPLATE_* tokens. This way it's possible for the preparser to report a nicer error (unterminated template literal, unterminated template expression), rather than "Illegal token".
I was hoping Dimitri or Andreas would review this too.
On 2014/11/13 16:49:02, arv wrote: > I was hoping Dimitri or Andreas would review this too. Sorry, Dmitry.
Looks good, just a few nits and some more tests. https://codereview.chromium.org/663683006/diff/600001/src/parser.cc File src/parser.cc (right): https://codereview.chromium.org/663683006/diff/600001/src/parser.cc#newcode5054 src/parser.cc:5054: #define COOKED_STRING(i) cooked_strings->at(i) Nit: please remove these macros, they don't seem worth it. https://codereview.chromium.org/663683006/diff/600001/src/parser.cc#newcode5077 src/parser.cc:5077: EXPR(i), POS(i)), POS(i)); Nit: indentation? https://codereview.chromium.org/663683006/diff/600001/src/parser.cc#newcode5098 src/parser.cc:5098: Expression* expr = factory()->NewCallRuntime( Nit: rename from 'expr' to 'call_site' https://codereview.chromium.org/663683006/diff/600001/src/parser.cc#newcode5150 src/parser.cc:5150: if (raw_chars[from_index + 1] == '\n') { Perhaps add a comment explaining that it is actually safe to access index+1 here without checking against the length first. https://codereview.chromium.org/663683006/diff/600001/src/parser.cc#newcode5153 src/parser.cc:5153: ch = '\n'; Nit: put this first. https://codereview.chromium.org/663683006/diff/600001/src/preparser.h File src/preparser.h (right): https://codereview.chromium.org/663683006/diff/600001/src/preparser.h#newcode... src/preparser.h:2925: // If we reach a TEMPLATE_TAIL first, we are parsing a NoSubstitutionTemplate. Nit: put this alternative into a conditional instead of the other, since it is much shorter. https://codereview.chromium.org/663683006/diff/600001/test/cctest/test-parsin... File test/cctest/test-parsing.cc (right): https://codereview.chromium.org/663683006/diff/600001/test/cctest/test-parsin... test/cctest/test-parsing.cc:4348: } Add tests for some of the different InputElementTemplateTail alternatives, e.g. "`foo${a /* comment */}bar`" "`foo${a \n}bar`" "`foo${a // comment\n}bar`" etc. https://codereview.chromium.org/663683006/diff/600001/test/cctest/test-parsin... test/cctest/test-parsing.cc:4397: } We should also have sync test cases where the script ends in the middle of a substitution, e.g. "`foo${a", or where a substitution expression is otherwise ill-formed, e.g., "`foo${5 if}bar`" or "`foo${f(}bar`".
Thanks for the look, I've re-arranged code a bit to address the style issues, and added some extra test cases. https://codereview.chromium.org/663683006/diff/600001/src/parser.cc File src/parser.cc (right): https://codereview.chromium.org/663683006/diff/600001/src/parser.cc#newcode5150 src/parser.cc:5150: if (raw_chars[from_index + 1] == '\n') { On 2014/11/14 12:28:07, rossberg wrote: > Perhaps add a comment explaining that it is actually safe to access index+1 here > without checking against the length first. Hmm, I'm not sure this is guaranteed to be safe, actually. Adding a check.
lgtm https://codereview.chromium.org/663683006/diff/640001/src/parser.cc File src/parser.cc (right): https://codereview.chromium.org/663683006/diff/640001/src/parser.cc#newcode5152 src/parser.cc:5152: if ((from_index + 1) < length && raw_chars[from_index + 1] == '\n') { Nit: redundant parens.
Can you also update the BUILD.gn file?
On 2014/11/14 14:40:02, arv wrote: > Can you also update the BUILD.gn file? Done (I think), does that look about right?
The CQ bit was checked by arv@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/663683006/670001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: v8_linux64_asan_rel on tryserver.v8 (http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel/builds/28) v8_linux64_rel on tryserver.v8 (http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel/builds/1237) v8_linux_rel on tryserver.v8 (http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel/builds/1220) v8_win64_compile_rel on tryserver.v8 (http://build.chromium.org/p/tryserver.v8/builders/v8_win64_compile_rel/builds...)
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/663683006/710001
Message was sent while issue was closed.
Committed patchset #33 (id:710001)
Message was sent while issue was closed.
brucedawson@chromium.org changed reviewers: + brucedawson@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/663683006/diff/710001/src/preparser.h File src/preparser.h (right): https://codereview.chromium.org/663683006/diff/710001/src/preparser.h#newcode... src/preparser.h:2816: int pos = peek_position(); While running VC++'s /analyze on Chrome I got a warning for this line of code, pointing out that the definition of 'pos' shadows another definition in the outer scope. These warnings are fairly common, but this one looks potentially wrong. The use of 'pos' in the ReportMessageAt call a few lines down looks like it might be intended to use the outer-scope pos. At the very least the intent is not clear. Any thoughts?
Message was sent while issue was closed.
https://codereview.chromium.org/663683006/diff/710001/src/preparser.h File src/preparser.h (right): https://codereview.chromium.org/663683006/diff/710001/src/preparser.h#newcode... src/preparser.h:2816: int pos = peek_position(); On 2014/12/09 22:37:13, brucedawson wrote: > While running VC++'s /analyze on Chrome I got a warning for this line of code, > pointing out that the definition of 'pos' shadows another definition in the > outer scope. These warnings are fairly common, but this one looks potentially > wrong. The use of 'pos' in the ReportMessageAt call a few lines down looks like > it might be intended to use the outer-scope pos. At the very least the intent is > not clear. > > Any thoughts? The usage looks good (we are reporting the error from pos to peek_position() which changes after we parse an expression.
Message was sent while issue was closed.
Message was sent while issue was closed.
https://codereview.chromium.org/663683006/diff/710001/src/preparser.h File src/preparser.h (right): https://codereview.chromium.org/663683006/diff/710001/src/preparser.h#newcode... src/preparser.h:2816: int pos = peek_position(); On 2014/12/09 22:40:49, Dmitry Lomov (chromium) wrote: > On 2014/12/09 22:37:13, brucedawson wrote: > > While running VC++'s /analyze on Chrome I got a warning for this line of code, > > pointing out that the definition of 'pos' shadows another definition in the > > outer scope. These warnings are fairly common, but this one looks potentially > > wrong. The use of 'pos' in the ReportMessageAt call a few lines down looks > like > > it might be intended to use the outer-scope pos. At the very least the intent > is > > not clear. > > > > Any thoughts? > > The usage looks good (we are reporting the error from pos to peek_position() > which changes after we parse an expression. We could rename it to avoid the warning, if that makes sense
Message was sent while issue was closed.
Renaming it would be nice, depending on the tradeoff you want between code clarity and avoiding churn. Glad to hear the code is correct.
Message was sent while issue was closed.
On 2014/12/09 22:42:15, caitp wrote: > https://codereview.chromium.org/663683006/diff/710001/src/preparser.h > File src/preparser.h (right): > > https://codereview.chromium.org/663683006/diff/710001/src/preparser.h#newcode... > src/preparser.h:2816: int pos = peek_position(); > On 2014/12/09 22:40:49, Dmitry Lomov (chromium) wrote: > > On 2014/12/09 22:37:13, brucedawson wrote: > > > While running VC++'s /analyze on Chrome I got a warning for this line of > code, > > > pointing out that the definition of 'pos' shadows another definition in the > > > outer scope. These warnings are fairly common, but this one looks > potentially > > > wrong. The use of 'pos' in the ReportMessageAt call a few lines down looks > > like > > > it might be intended to use the outer-scope pos. At the very least the > intent > > is > > > not clear. > > > > > > Any thoughts? > > > > The usage looks good (we are reporting the error from pos to peek_position() > > which changes after we parse an expression. > > We could rename it to avoid the warning, if that makes sense Up to you Caitlin :) I am fine either way |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
