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#newcode2816
src/preparser.h:2816: int pos = peek_position();
While running VC++'s /analyze on Chrome I got a warning for this
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#newcode2816
src/preparser.h:2816: int pos = peek_position();
On 2014/12/09 22:37:13, brucedawson wrote:
While running VC++'s
https://codereview.chromium.org/663683006/
--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups v8-dev group.
To unsubscribe from this group and stop receiving emails from it, send
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#newcode2816
src/preparser.h:2816: int pos = peek_position();
On 2014/12/09 22:40:49, Dmitry Lomov (chromium) wrote:
On
Renaming it would be nice, depending on the tradeoff you want between code
clarity and avoiding churn.
Glad to hear the code is correct.
https://codereview.chromium.org/663683006/
--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
---
You received this
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#newcode2816
src/preparser.h:2816: int pos = peek_position();
On 2014/12/09 22:40:49,
Looks good, just a few nits and some more tests.
https://codereview.chromium.org/663683006/diff/61/src/parser.cc
File src/parser.cc (right):
https://codereview.chromium.org/663683006/diff/61/src/parser.cc#newcode5054
src/parser.cc:5054: #define COOKED_STRING(i) cooked_strings-at(i)
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/61/src/parser.cc
File src/parser.cc (right):
https://codereview.chromium.org/663683006/diff/61/src/parser.cc#newcode5150
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?
https://codereview.chromium.org/663683006/
--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups v8-dev group.
To unsubscribe from this group
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?
https://codereview.chromium.org/663683006/
--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
---
You received this message because you
CQ is trying da patch. Follow status at
https://chromium-cq-status.appspot.com/patch-status/663683006/670001
https://codereview.chromium.org/663683006/
--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
---
You received this message because you are
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
CQ is trying da patch. Follow status at
https://chromium-cq-status.appspot.com/patch-status/663683006/710001
https://codereview.chromium.org/663683006/
--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
---
You received this message because you are
Committed patchset #33 (id:710001)
https://codereview.chromium.org/663683006/
--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups v8-dev group.
To unsubscribe from this group and
caitp, should we land this now?
https://codereview.chromium.org/663683006/
--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups v8-dev group.
To unsubscribe from this group and
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
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
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
I was hoping Dimitri or Andreas would review this too.
https://codereview.chromium.org/663683006/
--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups v8-dev group.
To unsubscribe
On 2014/11/13 16:49:02, arv wrote:
I was hoping Dimitri or Andreas would review this too.
Sorry, Dmitry.
https://codereview.chromium.org/663683006/
--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
---
You received this message because you are
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!
Oh and pls change the description of the CL to something more descriptive
as a
preparation step for landing this soon!
https://codereview.chromium.org/663683006/
--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
---
You received this message because you
Can somebody (arv@?) have a look at src/harmony-templates.js, I can't say
whether it's correct or not.
https://codereview.chromium.org/663683006/
--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to
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/41/src/parser.cc
File src/parser.cc (right):
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
https://codereview.chromium.org/663683006/diff/41/src/parser.cc
File src/parser.cc (right):
https://codereview.chromium.org/663683006/diff/41/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
On 2014/11/12 13:41:14, marja wrote:
https://codereview.chromium.org/663683006/diff/41/src/parser.cc
File src/parser.cc (right):
https://codereview.chromium.org/663683006/diff/41/src/parser.cc#newcode5093
src/parser.cc:5093: for (int i = 0; i cookedStrings-length(); ++i) {
On
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.js#newcode16
src/harmony-templates.js:16: DefineArrayProperty(siteObj, prop,
ToPropertyDescriptor({
I
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.js#newcode16
src/harmony-templates.js:16: DefineArrayProperty(siteObj, prop,
ToPropertyDescriptor({
A few more tests would be useful to prevent regressions.
https://codereview.chromium.org/663683006/diff/51/test/mjsunit/es6/templates.js
File test/mjsunit/es6/templates.js (right):
https://codereview.chromium.org/663683006/diff/51/test/mjsunit/es6/templates.js#newcode38
https://codereview.chromium.org/663683006/diff/51/test/mjsunit/es6/templates.js
File test/mjsunit/es6/templates.js (right):
https://codereview.chromium.org/663683006/diff/51/test/mjsunit/es6/templates.js#newcode38
test/mjsunit/es6/templates.js:38: (function testLineCondition() {
On
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/templates.js
File test/mjsunit/es6/templates.js (right):
https://codereview.chromium.org/663683006/diff/520001/test/mjsunit/es6/templates.js
File test/mjsunit/es6/templates.js (right):
https://codereview.chromium.org/663683006/diff/520001/test/mjsunit/es6/templates.js#newcode134
test/mjsunit/es6/templates.js:134: (function(s) { calls++;
LGTM
https://codereview.chromium.org/663683006/diff/540001/test/mjsunit/harmony/templates.js
File test/mjsunit/harmony/templates.js (right):
https://codereview.chromium.org/663683006/diff/540001/test/mjsunit/harmony/templates.js#newcode297
test/mjsunit/harmony/templates.js:297:
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?
https://codereview.chromium.org/663683006/
--
--
v8-dev mailing list
v8-dev@googlegroups.com
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):
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
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
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,
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 :)
https://codereview.chromium.org/663683006/
--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
---
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
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.js#newcode14
src/harmony-templates.js:14: for (; index count; ++index) {
why not?
for
https://codereview.chromium.org/663683006/
--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups v8-dev group.
To unsubscribe from this group and stop receiving emails from it, send
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
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):
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):
Added some comments to explain the parsing and scanning of template
literals, I
hope that makes it easier =)
https://codereview.chromium.org/663683006/
--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
---
You received this message because you are
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
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.
https://codereview.chromium.org/663683006/
--
--
v8-dev mailing
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
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
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
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,
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
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
The important part is that two call sites that have the same structure have
the same identity.
Here is an example where using the call site object as a key in cache
allows optimizations in Traceur:
https://github.com/google/traceur-compiler/blob/master/src/codegeneration/PlaceholderParser.js#L103
On 7 November 2014 15:52, Erik Arvidsson a...@chromium.org wrote:
The important part is that two call sites that have the same structure have
the same identity.
Yes, but wouldn't it be more adequate to key on the structural information?
/Andreas
Here is an example where using the call site
Yes. That is what I think the spec should do [*]. If this is done in the VM
then a lot of churn can be prevented. It also makes user code simpler but
that might not be a strong argument.
[*] Structure + global.
On Fri, Nov 7, 2014 at 10:04 AM, 'Andreas Rossberg' via v8-dev
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
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
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
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),
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.
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/mjsunit/es6/templates.js
File test/mjsunit/es6/templates.js (right):
https://codereview.chromium.org/663683006/diff/180001/test/mjsunit/es6/templates.js#newcode28
test/mjsunit/es6/templates.js:28: // Multi-line templates
On 2014/11/07
https://codereview.chromium.org/663683006/diff/180001/test/mjsunit/es6/templates.js
File test/mjsunit/es6/templates.js (right):
https://codereview.chromium.org/663683006/diff/180001/test/mjsunit/es6/templates.js#newcode28
test/mjsunit/es6/templates.js:28: // Multi-line templates
On 2014/11/07
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
Just FWIW,
https://www.dropbox.com/s/5f9sq0tqifkmsvc/spidermonkey_does_it_wrong_too.png?dl=0
SpiderMonkey is not caching the CallSite either, so maybe it's not a
blocker for
landing. It would be good to fix this, though.
https://codereview.chromium.org/663683006/
--
--
v8-dev mailing list
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.
https://codereview.chromium.org/663683006/
--
--
v8-dev mailing list
https://codereview.chromium.org/663683006/diff/120001/test/mjsunit/es6/templates.js
File test/mjsunit/es6/templates.js (right):
https://codereview.chromium.org/663683006/diff/120001/test/mjsunit/es6/templates.js#newcode76
test/mjsunit/es6/templates.js:76: // assertEquals(, s.raw[0]);
If I could
https://codereview.chromium.org/663683006/diff/120001/test/mjsunit/es6/templates.js
File test/mjsunit/es6/templates.js (right):
https://codereview.chromium.org/663683006/diff/120001/test/mjsunit/es6/templates.js#newcode76
test/mjsunit/es6/templates.js:76: // assertEquals(, s.raw[0]);
On
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
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_3vlPCWvSs2dby0aq8Hjah8gI/edit?usp=sharing
https://codereview.chromium.org/663683006/
--
--
v8-dev mailing list
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
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
We should really have this discussion outside this CL.
1. For non tagged template literals, we should do the desugaring in the
parser. That way we need no code gen for it.
2. Is complicated because the CallSite object has to be the same every time
the template literal is executed.
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
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/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
80 matches
Mail list logo
