after about a year since the last try, here's another (simpler) go at the latest
version of the proposal.
There's some messy bits, to make the right-associativity of the `**` token work,
probably a better way to do that.
https://codereview.chromium.org/1678303002/diff/20001/test/webkit/fast/js/par...
File test/webkit/fast/js/parser-syntax-check-expected.txt (right):
https://codereview.chromium.org/1678303002/diff/20001/test/webkit/fast/js/par...
test/webkit/fast/js/parser-syntax-check-expected.txt:61: FAIL Valid: "function
f() { ++ -- ++ a }" should NOT throw
On 2016/02/08 22:09:44, caitp wrote:
> The grammar change in the latest version of the proposal might be a bit
> dangerous because of these ReferenceErrors becoming early errors /cc rwaldron
> (but I assume people have been okay with this so far?)
In implementations today, it's a ReferenceError at runtime, rather than a
SyntaxError (although it might actually just be an early ReferenceError)
caitp (gmail)
> I'm concerned with this: > > > Implements Stage 4 proposal from > http://rwaldron.github.io/exponentiation-operator/, ...
> I'm concerned with this:
>
> > Implements Stage 4 proposal from
> http://rwaldron.github.io/exponentiation-operator/,
>
> Once I started writing the actual spec patch for
http://github.com/tc39/ecma262,
> I didn't back-port any changes that were made during review with Brian and
> Andre. I know that the evaluation order is wrong here:
> http://rwaldron.github.io/exponentiation-operator/, as a result of an attempt
to
> "over optimize" the spec language.
Hmm, could you link me to some of these discussions? I believe right now this is
mostly or entirely compatible with the implementation in gecko
On 2016/02/09 23:09:09, caitp wrote:
>
https://codereview.chromium.org/1678303002/diff/20001/test/webkit/fast/js/par...
> File test/webkit/fast/js/parser-syntax-check-expected.txt (right):
>
>
https://codereview.chromium.org/1678303002/diff/20001/test/webkit/fast/js/par...
> test/webkit/fast/js/parser-syntax-check-expected.txt:61: FAIL Valid:
"function
> f() { ++ -- ++ a }" should NOT throw
> On 2016/02/08 22:09:44, caitp wrote:
> > The grammar change in the latest version of the proposal might be a bit
> > dangerous because of these ReferenceErrors becoming early errors /cc
rwaldron
> > (but I assume people have been okay with this so far?)
>
> In implementations today, it's a ReferenceError at runtime, rather than a
> SyntaxError (although it might actually just be an early ReferenceError)
In SpiderMonkey it's a SyntaxError (which is what I would expect).
caitp (gmail)
On 2016/02/09 23:34:32, Rick Waldron wrote: > On 2016/02/09 23:09:09, caitp wrote: > > > ...
On 2016/02/09 23:34:32, Rick Waldron wrote:
> On 2016/02/09 23:09:09, caitp wrote:
> >
>
https://codereview.chromium.org/1678303002/diff/20001/test/webkit/fast/js/par...
> > File test/webkit/fast/js/parser-syntax-check-expected.txt (right):
> >
> >
>
https://codereview.chromium.org/1678303002/diff/20001/test/webkit/fast/js/par...
> > test/webkit/fast/js/parser-syntax-check-expected.txt:61: FAIL Valid:
> "function
> > f() { ++ -- ++ a }" should NOT throw
> > On 2016/02/08 22:09:44, caitp wrote:
> > > The grammar change in the latest version of the proposal might be a bit
> > > dangerous because of these ReferenceErrors becoming early errors /cc
> rwaldron
> > > (but I assume people have been okay with this so far?)
> >
> > In implementations today, it's a ReferenceError at runtime, rather than a
> > SyntaxError (although it might actually just be an early ReferenceError)
>
> In SpiderMonkey it's a SyntaxError (which is what I would expect).
It's a WebKit test, which checks if eval() throws a SyntaxError or not
Rick Waldron
On 2016/02/09 23:12:48, caitp wrote: > > I'm concerned with this: > > > > ...
On 2016/02/09 23:12:48, caitp wrote:
> > I'm concerned with this:
> >
> > > Implements Stage 4 proposal from
> > http://rwaldron.github.io/exponentiation-operator/,
> >
> > Once I started writing the actual spec patch for
> http://github.com/tc39/ecma262,
> > I didn't back-port any changes that were made during review with Brian and
> > Andre. I know that the evaluation order is wrong here:
> > http://rwaldron.github.io/exponentiation-operator/, as a result of an
attempt
> to
> > "over optimize" the spec language.
>
> Hmm, could you link me to some of these discussions? I believe right now this
is
> mostly or entirely compatible with the implementation in gecko
Well, one discussion occurred over google hangout :P I've backported all of the
differences to the feature spec:
http://rwaldron.github.io/exponentiation-operator/
https://codereview.chromium.org/1678303002/diff/20001/src/parsing/parser-base.h
File src/parsing/parser-base.h (right):
https://codereview.chromium.org/1678303002/diff/20001/src/parsing/parser-base...
src/parsing/parser-base.h:2245: // somehow
On 2016/02/11 08:16:50, Dan Ehrenberg wrote:
> Seems like a good thing to do before submitting this patch. Does this code
> ensure that there's a SyntaxError in the -x**y case?
Yes it does-- the model for existing right associativity in v8 is just hard to
understand how to model this, since it's the only right associative binary op,
and doesn't fit well with the preexisting model. I'm not sure what the best
refactoring is yet, so i went with the simple hack first.
Could make it clearer by having a variable like `asspciativity` set to
`kRightAssociative` or something of the op is **, and change how the loop works
based on that. It's the same hack, just expressed more clearly?
I am sure people have preferences for this, but it is a very unusual behavior
for a binary op in js, so i wasn't sure how we wanted to do it, thus wanted
feedback on it
caitp (gmail)
https://codereview.chromium.org/1678303002/diff/20001/src/parsing/parser-base.h File src/parsing/parser-base.h (right): https://codereview.chromium.org/1678303002/diff/20001/src/parsing/parser-base.h#newcode2245 src/parsing/parser-base.h:2245: // somehow On 2016/02/11 12:57:24, caitp wrote: > On ...
https://codereview.chromium.org/1678303002/diff/20001/src/parsing/parser-base.h
File src/parsing/parser-base.h (right):
https://codereview.chromium.org/1678303002/diff/20001/src/parsing/parser-base...
src/parsing/parser-base.h:2245: // somehow
On 2016/02/11 12:57:24, caitp wrote:
> On 2016/02/11 08:16:50, Dan Ehrenberg wrote:
> > Seems like a good thing to do before submitting this patch. Does this code
> > ensure that there's a SyntaxError in the -x**y case?
>
> Yes it does-- the model for existing right associativity in v8 is just hard to
> understand how to model this, since it's the only right associative binary op,
> and doesn't fit well with the preexisting model. I'm not sure what the best
> refactoring is yet, so i went with the simple hack first.
>
> Could make it clearer by having a variable like `asspciativity` set to
> `kRightAssociative` or something of the op is **, and change how the loop
works
> based on that. It's the same hack, just expressed more clearly?
>
> I am sure people have preferences for this, but it is a very unusual behavior
> for a binary op in js, so i wasn't sure how we wanted to do it, thus wanted
> feedback on it
I've simplified the unary-op SyntaxError code and refactored this slightly, to
look a little bit cleaner --- but it's really the same hack and adds a few lines
of code
Glad to see it passes the tests; just a couple comments. https://codereview.chromium.org/1678303002/diff/180001/src/parsing/parser.cc File src/parsing/parser.cc (right): ...
addressed, sorry it took so long @_@ https://codereview.chromium.org/1678303002/diff/180001/src/parsing/parser.cc File src/parsing/parser.cc (right): https://codereview.chromium.org/1678303002/diff/180001/src/parsing/parser.cc#newcode5641 src/parsing/parser.cc:5641: if (op ...
4 years, 12 months ago
(2016-03-11 15:54:19 UTC)
#15
https://codereview.chromium.org/1678303002/diff/260001/src/parsing/parser.cc File src/parsing/parser.cc (right): https://codereview.chromium.org/1678303002/diff/260001/src/parsing/parser.cc#newcode5494 src/parsing/parser.cc:5494: // backend support for exponent op, some day On ...
4 years, 11 months ago
(2016-03-14 21:05:14 UTC)
#18
Sorry I didn't say before, but there are a few other places where you could ...
4 years, 11 months ago
(2016-03-14 21:13:25 UTC)
#19
Sorry I didn't say before, but there are a few other places where you could
improve test coverage.
https://codereview.chromium.org/1678303002/diff/260001/src/parsing/parser.cc
File src/parsing/parser.cc (right):
https://codereview.chromium.org/1678303002/diff/260001/src/parsing/parser.cc#...
src/parsing/parser.cc:5494: // backend support for exponent op, some day
On 2016/03/14 at 21:05:14, caitp wrote:
> On 2016/03/14 20:48:46, Dan Ehrenberg wrote:
> > I'm not so convinced that this is a bad way to go, long-term. Just curious,
why
> > is the TODO assigned to bmeurer?
>
> eh, it could be more `TODO(backend-knowledgable-people)`, I guess --- name is
just picked due to tech lead status
>
> There are some advantages to implementing this in backends, such as not
needing to allocate a temp variable for the holder of the property, avoiding
ToNumber() in common cases, etc. It's not an urgent thing, though, and most
applications probably won't notice the difference.
If you want to insert a TODO, it should probably be assigned to yourself. See
https://google.github.io/styleguide/cppguide.html#TODO_Comments .
I'm not convinced that there'd be much benefit to putting it in the backends,
anyway--we're doing more things these days with desugaring, even though it
increases the number of temp variables, and we also have lots of calls to
ToNumber all over the place that we make do with performance-wise. I don't see
anything crazy about it. I'd rather we increase the separation of concerns
between the frontend and backends, rather than make the backends more
complicated, whenever possible.
https://codereview.chromium.org/1678303002/diff/280001/test/cctest/test-parsi...
File test/cctest/test-parsing.cc (right):
https://codereview.chromium.org/1678303002/diff/280001/test/cctest/test-parsi...
test/cctest/test-parsing.cc:7339: "x-- ** 10",
How about some successful parse tests here?
https://codereview.chromium.org/1678303002/diff/280001/test/mjsunit/harmony/e...
File test/mjsunit/harmony/exponentiation-operator.js (right):
https://codereview.chromium.org/1678303002/diff/280001/test/mjsunit/harmony/e...
test/mjsunit/harmony/exponentiation-operator.js:197: assertEquals(NaN, (-16) **
-0.5);
Tests for **= functioning correctly? How about verifying that overwriting
Math.pow doesn't have an effect on anything?
caitp (gmail)
https://codereview.chromium.org/1678303002/diff/280001/test/cctest/test-parsing.cc File test/cctest/test-parsing.cc (right): https://codereview.chromium.org/1678303002/diff/280001/test/cctest/test-parsing.cc#newcode7339 test/cctest/test-parsing.cc:7339: "x-- ** 10", On 2016/03/14 21:13:25, Dan Ehrenberg wrote: ...
4 years, 11 months ago
(2016-03-14 22:52:12 UTC)
#20
https://codereview.chromium.org/1678303002/diff/280001/test/cctest/test-parsi...
File test/cctest/test-parsing.cc (right):
https://codereview.chromium.org/1678303002/diff/280001/test/cctest/test-parsi...
test/cctest/test-parsing.cc:7339: "x-- ** 10",
On 2016/03/14 21:13:25, Dan Ehrenberg wrote:
> How about some successful parse tests here?
I'm not sure what you mean, this comment is put in the list of successful parse
tests
https://codereview.chromium.org/1678303002/diff/280001/test/mjsunit/harmony/e...
File test/mjsunit/harmony/exponentiation-operator.js (right):
https://codereview.chromium.org/1678303002/diff/280001/test/mjsunit/harmony/e...
test/mjsunit/harmony/exponentiation-operator.js:197: assertEquals(NaN, (-16) **
-0.5);
On 2016/03/14 21:13:25, Dan Ehrenberg wrote:
> Tests for **= functioning correctly? How about verifying that overwriting
> Math.pow doesn't have an effect on anything?
ah, I've added some tests to verify that overriding Math.pow doesn't affect the
algorithm, some operation ordering tests using Proxy, assertions about throwing
with bad LHS in assignment contexts (with a TODO to throw an early reference
error when LHS is a call expression, if we can ever really fix that). Most of
these aren't covered in test262, so maybe that's something Rick could add.
Dan Ehrenberg
https://codereview.chromium.org/1678303002/diff/300001/src/parsing/parser.cc File src/parsing/parser.cc (right): https://codereview.chromium.org/1678303002/diff/300001/src/parsing/parser.cc#newcode5494 src/parsing/parser.cc:5494: // avoid ToNumber() when possible (the common cases). Please ...
4 years, 11 months ago
(2016-03-15 01:00:57 UTC)
#21
https://codereview.chromium.org/1678303002/diff/300001/src/parsing/parser.cc File src/parsing/parser.cc (right): https://codereview.chromium.org/1678303002/diff/300001/src/parsing/parser.cc#newcode5494 src/parsing/parser.cc:5494: // avoid ToNumber() when possible (the common cases). On ...
4 years, 11 months ago
(2016-03-15 01:03:05 UTC)
#22
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1678303002/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1678303002/320001
4 years, 11 months ago
(2016-03-18 13:29:38 UTC)
#26
https://codereview.chromium.org/1678303002/diff/320001/src/parsing/parser-base.h File src/parsing/parser-base.h (right): https://codereview.chromium.org/1678303002/diff/320001/src/parsing/parser-base.h#newcode141 src/parsing/parser-base.h:141: #undef ALLOW_ACCESSORS Shouldn't there be an #undef SCANNER_ACCESSORS here?
4 years, 11 months ago
(2016-03-21 08:51:49 UTC)
#31
Issue 1678303002: [es7] implement exponentiation operator proposal
(Closed)
Created 5 years ago by caitp (gmail)
Modified 4 years, 11 months ago
Reviewers: Dan Ehrenberg, adamk, rossberg, nickie
Base URL: https://chromium.googlesource.com/v8/v8.git@master
Comments: 28