Non-pattern rewriting revisited

This patch implements an alternative approach to the rewriting
of non-pattern expressions, according to the (internally circulated)
design document.  Details to be provided here.

1.  RewritableAssignmentExpression has been renamed to RewritableExpression.
    It is a wrapper for AST nodes that wait for some potential rewriting
    (that may or may not happen).  Also, Is... and As... macros now see
    through RewritableExpressions.

2.  The function state keeps a list of rewritable expressions that must be
    rewritten only if they are used as non-pattern expressions.

3.  Expression classifiers are now templates, parameterized by parser
    traits.  They keep some additional state: a pointer to the list of
    non-pattern rewritable expressions.  It is important that expression
    classifiers be used strictly in a stack fashion, from now on.

4.  The RewriteNonPattern function has been simplified.

BUG=chromium:579913
LOG=N

Committed: https://crrev.com/7f5c864a6faf2b957b7273891e143b9bde35487c
Cr-Commit-Position: refs/heads/master@{#34154}

Committed: https://crrev.com/ed665880416e5ece6561ab128b506896e2079809
Cr-Commit-Position: refs/heads/master@{#34162}

[nickie, 2016-02-17 15:01:55]

Description was changed from

==========
Non-pattern rewriting revisited, 2

This patch implements an alternative approach to the rewriting
of non-pattern expressions, according to the (internally circulated)
design document.  Details to be provided here.

Alternative attempt 2 (429dc8b, rm-nested).
==========

to

==========
Non-pattern rewriting revisited, 2

This patch implements an alternative approach to the rewriting
of non-pattern expressions, according to the (internally circulated)
design document.  Details to be provided here.

Alternative attempt 2 (429dc8b, rm-nested).

BUG=
==========

[nickie, 2016-02-17 15:03:42]

Description was changed from

==========
Non-pattern rewriting revisited, 2

This patch implements an alternative approach to the rewriting
of non-pattern expressions, according to the (internally circulated)
design document.  Details to be provided here.

Alternative attempt 2 (429dc8b, rm-nested).

BUG=
==========

to

==========
Non-pattern rewriting revisited

This patch implements an alternative approach to the rewriting
of non-pattern expressions, according to the (internally circulated)
design document.  Details to be provided here.

1.  RewritableAssignmentExpression has been renamed to RewritableExpression.
    It is a wrapper for AST nodes that wait for some potential rewriting
    (that may or may not happen).  Also, Is... and As... macros now see
    through RewritableExpressions.

2.  The function state keeps a list of rewritable expressions that must be
    rewritten only if they are used as non-pattern expressions.

3.  Expression classifiers are now templates, parameterized by parser
    traits.  They keep some additional state: a pointer to the list of
    non-pattern rewritable expressions.  It is important that expression
    classifiers be used strictly in a stack fashion, from now on.

4.  The RewriteNonPattern function has been simplified.

BUG=chromium:579913
==========

[nickie, 2016-02-17 15:05:19]

Description was changed from

==========
Non-pattern rewriting revisited

This patch implements an alternative approach to the rewriting
of non-pattern expressions, according to the (internally circulated)
design document.  Details to be provided here.

1.  RewritableAssignmentExpression has been renamed to RewritableExpression.
    It is a wrapper for AST nodes that wait for some potential rewriting
    (that may or may not happen).  Also, Is... and As... macros now see
    through RewritableExpressions.

2.  The function state keeps a list of rewritable expressions that must be
    rewritten only if they are used as non-pattern expressions.

3.  Expression classifiers are now templates, parameterized by parser
    traits.  They keep some additional state: a pointer to the list of
    non-pattern rewritable expressions.  It is important that expression
    classifiers be used strictly in a stack fashion, from now on.

4.  The RewriteNonPattern function has been simplified.

BUG=chromium:579913
==========

to

==========
This patch implements an alternative approach to the rewriting
of non-pattern expressions, according to the (internally circulated)
design document.  Details to be provided here.

1.  RewritableAssignmentExpression has been renamed to RewritableExpression.
    It is a wrapper for AST nodes that wait for some potential rewriting
    (that may or may not happen).  Also, Is... and As... macros now see
    through RewritableExpressions.

2.  The function state keeps a list of rewritable expressions that must be
    rewritten only if they are used as non-pattern expressions.

3.  Expression classifiers are now templates, parameterized by parser
    traits.  They keep some additional state: a pointer to the list of
    non-pattern rewritable expressions.  It is important that expression
    classifiers be used strictly in a stack fashion, from now on.

4.  The RewriteNonPattern function has been simplified.

BUG=chromium:579913
==========

[nickie, 2016-02-17 15:05:19]

nikolaos@chromium.org changed reviewers:
+ bmeurer@chromium.org, rossberg@chromium.org, titzer@chromium.org

[nickie, 2016-02-17 15:06:20]

https://codereview.chromium.org/1702063002/diff/1/src/parsing/parser-base.h
File src/parsing/parser-base.h (left):

https://codereview.chromium.org/1702063002/diff/1/src/parsing/parser-base.h#o...
src/parsing/parser-base.h:248: 
This is not deleted, it is made private.

[rossberg, 2016-02-18 12:11:32]

Looks good mostly nits.

https://codereview.chromium.org/1702063002/diff/1/src/ast/ast.h
File src/ast/ast.h (right):

https://codereview.chromium.org/1702063002/diff/1/src/ast/ast.h#newcode3534
src/ast/ast.h:3534: AstNode::k##type != AstNode::kRewritableExpression)         
         \
I assume compilers are smart enough to fold away this constant condition.

https://codereview.chromium.org/1702063002/diff/1/src/ast/ast.h#newcode3546
src/ast/ast.h:3546: reinterpret_cast<const
RewritableExpression*>(this)->expression();  \
I'm surprised C++ does not barf on the cast in line 3549 with the const here. In
any case, the const seems redundant.

https://codereview.chromium.org/1702063002/diff/1/src/parsing/expression-clas...
File src/parsing/expression-classifier.h (right):

https://codereview.chromium.org/1702063002/diff/1/src/parsing/expression-clas...
src/parsing/expression-classifier.h:295: void Accumulate(ExpressionClassifier*
inner,
It's unfortunate that accumulation now has to mutate the inner classifier. I
wonder if there is a way of avoiding that. Can't think of any, though...

https://codereview.chromium.org/1702063002/diff/1/src/parsing/expression-clas...
src/parsing/expression-classifier.h:352: V8_INLINE void Discard() {
Can we give this a more descriptive name? Say, DiscardNonPatterns?

https://codereview.chromium.org/1702063002/diff/1/src/parsing/pattern-rewrite...
File src/parsing/pattern-rewriter.cc (right):

https://codereview.chromium.org/1702063002/diff/1/src/parsing/pattern-rewrite...
src/parsing/pattern-rewriter.cc:328: // If this is not a destructive
assignment...
Nit: s/destructive/destructuring/

[nickie, 2016-02-18 12:53:17]

https://codereview.chromium.org/1702063002/diff/1/src/ast/ast.h
File src/ast/ast.h (right):

https://codereview.chromium.org/1702063002/diff/1/src/ast/ast.h#newcode3534
src/ast/ast.h:3534: AstNode::k##type != AstNode::kRewritableExpression)         
         \
On 2016/02/18 12:11:31, rossberg wrote:
> I assume compilers are smart enough to fold away this constant condition.

Yes, I also assumed this.  If we have reason to believe this is not the case, we
can pretty easily define these macros separately for RewritableExpression and
for all other nodes in AST_NODE_LIST.  This will probably draw even more
attention to the fact that RewritableExpression is an exception of an AST node.

I'm happy either way.

https://codereview.chromium.org/1702063002/diff/1/src/ast/ast.h#newcode3546
src/ast/ast.h:3546: reinterpret_cast<const
RewritableExpression*>(this)->expression();  \
On 2016/02/18 12:11:32, rossberg wrote:
> I'm surprised C++ does not barf on the cast in line 3549 with the const here.
In
> any case, the const seems redundant.

I don't see why it should, but anyway.  The const is redundant (here, not in the
next one, line 3557), but I kept it for consistency.

https://codereview.chromium.org/1702063002/diff/1/src/parsing/expression-clas...
File src/parsing/expression-classifier.h (right):

https://codereview.chromium.org/1702063002/diff/1/src/parsing/expression-clas...
src/parsing/expression-classifier.h:295: void Accumulate(ExpressionClassifier*
inner,
On 2016/02/18 12:11:32, rossberg wrote:
> It's unfortunate that accumulation now has to mutate the inner classifier. I
> wonder if there is a way of avoiding that. Can't think of any, though...

Not much to say here.  You're right.  The alternative is to decouple the
classifier and have a new data structure for holding the extra information. 
This would require a lot of changes all over the parser.

https://codereview.chromium.org/1702063002/diff/1/src/parsing/expression-clas...
src/parsing/expression-classifier.h:352: V8_INLINE void Discard() {
On 2016/02/18 12:11:32, rossberg wrote:
> Can we give this a more descriptive name? Say, DiscardNonPatterns?

Yes, but in the (unrelated, sort of) upcoming patch this is also used for
discarding reported error messages.

The idea is that this method does what the object destructor will do, only you
don't have to wait for the end of the scope.  If you have a better name for it,
let's rename it.

https://codereview.chromium.org/1702063002/diff/1/src/parsing/pattern-rewrite...
File src/parsing/pattern-rewriter.cc (right):

https://codereview.chromium.org/1702063002/diff/1/src/parsing/pattern-rewrite...
src/parsing/pattern-rewriter.cc:328: // If this is not a destructive
assignment...
On 2016/02/18 12:11:32, rossberg wrote:
> Nit: s/destructive/destructuring/

Acknowledged.

[nickie, 2016-02-19 12:22:19]

Description was changed from

==========
This patch implements an alternative approach to the rewriting
of non-pattern expressions, according to the (internally circulated)
design document.  Details to be provided here.

1.  RewritableAssignmentExpression has been renamed to RewritableExpression.
    It is a wrapper for AST nodes that wait for some potential rewriting
    (that may or may not happen).  Also, Is... and As... macros now see
    through RewritableExpressions.

2.  The function state keeps a list of rewritable expressions that must be
    rewritten only if they are used as non-pattern expressions.

3.  Expression classifiers are now templates, parameterized by parser
    traits.  They keep some additional state: a pointer to the list of
    non-pattern rewritable expressions.  It is important that expression
    classifiers be used strictly in a stack fashion, from now on.

4.  The RewriteNonPattern function has been simplified.

BUG=chromium:579913
==========

to

==========
This patch implements an alternative approach to the rewriting
of non-pattern expressions, according to the (internally circulated)
design document.  Details to be provided here.

1.  RewritableAssignmentExpression has been renamed to RewritableExpression.
    It is a wrapper for AST nodes that wait for some potential rewriting
    (that may or may not happen).  Also, Is... and As... macros now see
    through RewritableExpressions.

2.  The function state keeps a list of rewritable expressions that must be
    rewritten only if they are used as non-pattern expressions.

3.  Expression classifiers are now templates, parameterized by parser
    traits.  They keep some additional state: a pointer to the list of
    non-pattern rewritable expressions.  It is important that expression
    classifiers be used strictly in a stack fashion, from now on.

4.  The RewriteNonPattern function has been simplified.

BUG=chromium:579913
LOG=N
==========

[rossberg, 2016-02-19 12:23:47]

LGTM

[nickie, 2016-02-19 12:26:38]

The CQ bit was checked by nikolaos@chromium.org

[commit-bot: I haz the power, 2016-02-19 12:26:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1702063002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1702063002/20001

[commit-bot: I haz the power, 2016-02-19 12:27:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-19 12:27:46]

Try jobs failed on following builders:
  v8_linux64_asan_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel/builds/...)
  v8_linux64_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/1633)
  v8_linux_dbg_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/1634)
  v8_linux_gcc_compile_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...)
  v8_linux_nodcheck_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel/build...)
  v8_linux_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/1630)
  v8_presubmit on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/11240)

[nickie, 2016-02-19 13:35:35]

The CQ bit was checked by nikolaos@chromium.org

[nickie, 2016-02-19 13:35:36]

The patchset sent to the CQ was uploaded after l-g-t-m from
rossberg@chromium.org
Link to the patchset: https://codereview.chromium.org/1702063002/#ps40001
(title: "Rebase")

[commit-bot: I haz the power, 2016-02-19 13:35:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1702063002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1702063002/40001

[commit-bot: I haz the power, 2016-02-19 13:43:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-19 13:43:26]

Try jobs failed on following builders:
  v8_presubmit on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/11249)

[Benedikt Meurer, 2016-02-19 13:45:03]

LGTM.

[titzer, 2016-02-19 14:00:41]

lgtm

[nickie, 2016-02-19 14:01:44]

The CQ bit was checked by nikolaos@chromium.org

[commit-bot: I haz the power, 2016-02-19 14:02:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1702063002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1702063002/40001

[commit-bot: I haz the power, 2016-02-19 14:03:41]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-02-19 14:04:29]

Description was changed from

==========
This patch implements an alternative approach to the rewriting
of non-pattern expressions, according to the (internally circulated)
design document.  Details to be provided here.

1.  RewritableAssignmentExpression has been renamed to RewritableExpression.
    It is a wrapper for AST nodes that wait for some potential rewriting
    (that may or may not happen).  Also, Is... and As... macros now see
    through RewritableExpressions.

2.  The function state keeps a list of rewritable expressions that must be
    rewritten only if they are used as non-pattern expressions.

3.  Expression classifiers are now templates, parameterized by parser
    traits.  They keep some additional state: a pointer to the list of
    non-pattern rewritable expressions.  It is important that expression
    classifiers be used strictly in a stack fashion, from now on.

4.  The RewriteNonPattern function has been simplified.

BUG=chromium:579913
LOG=N
==========

to

==========
This patch implements an alternative approach to the rewriting
of non-pattern expressions, according to the (internally circulated)
design document.  Details to be provided here.

1.  RewritableAssignmentExpression has been renamed to RewritableExpression.
    It is a wrapper for AST nodes that wait for some potential rewriting
    (that may or may not happen).  Also, Is... and As... macros now see
    through RewritableExpressions.

2.  The function state keeps a list of rewritable expressions that must be
    rewritten only if they are used as non-pattern expressions.

3.  Expression classifiers are now templates, parameterized by parser
    traits.  They keep some additional state: a pointer to the list of
    non-pattern rewritable expressions.  It is important that expression
    classifiers be used strictly in a stack fashion, from now on.

4.  The RewriteNonPattern function has been simplified.

BUG=chromium:579913
LOG=N

Committed: https://crrev.com/7f5c864a6faf2b957b7273891e143b9bde35487c
Cr-Commit-Position: refs/heads/master@{#34154}
==========

[commit-bot: I haz the power, 2016-02-19 14:04:30]

Patchset 3 (id:??) landed as
https://crrev.com/7f5c864a6faf2b957b7273891e143b9bde35487c
Cr-Commit-Position: refs/heads/master@{#34154}

[nickie, 2016-02-19 14:37:26]

Description was changed from

==========
This patch implements an alternative approach to the rewriting
of non-pattern expressions, according to the (internally circulated)
design document.  Details to be provided here.

1.  RewritableAssignmentExpression has been renamed to RewritableExpression.
    It is a wrapper for AST nodes that wait for some potential rewriting
    (that may or may not happen).  Also, Is... and As... macros now see
    through RewritableExpressions.

2.  The function state keeps a list of rewritable expressions that must be
    rewritten only if they are used as non-pattern expressions.

3.  Expression classifiers are now templates, parameterized by parser
    traits.  They keep some additional state: a pointer to the list of
    non-pattern rewritable expressions.  It is important that expression
    classifiers be used strictly in a stack fashion, from now on.

4.  The RewriteNonPattern function has been simplified.

BUG=chromium:579913
LOG=N

Committed: https://crrev.com/7f5c864a6faf2b957b7273891e143b9bde35487c
Cr-Commit-Position: refs/heads/master@{#34154}
==========

to

==========
This patch implements an alternative approach to the rewriting
of non-pattern expressions, according to the (internally circulated)
design document.  Details to be provided here.

1.  RewritableAssignmentExpression has been renamed to RewritableExpression.
    It is a wrapper for AST nodes that wait for some potential rewriting
    (that may or may not happen).  Also, Is... and As... macros now see
    through RewritableExpressions.

2.  The function state keeps a list of rewritable expressions that must be
    rewritten only if they are used as non-pattern expressions.

3.  Expression classifiers are now templates, parameterized by parser
    traits.  They keep some additional state: a pointer to the list of
    non-pattern rewritable expressions.  It is important that expression
    classifiers be used strictly in a stack fashion, from now on.

4.  The RewriteNonPattern function has been simplified.

BUG=chromium:579913
LOG=N

Committed: https://crrev.com/7f5c864a6faf2b957b7273891e143b9bde35487c
Cr-Commit-Position: refs/heads/master@{#34154}
==========

[Michael Achenbach, 2016-02-19 15:05:48]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/1712203002/ by machenbach@chromium.org.

The reason for reverting is: [Sheriff] This makes jsfunfuzz unhappy:
https://build.chromium.org/p/client.v8/builders/V8%20Fuzzer/builds/7681.

[nickie, 2016-02-19 15:29:51]

The CQ bit was checked by nikolaos@chromium.org

[nickie, 2016-02-19 15:29:51]

The patchset sent to the CQ was uploaded after l-g-t-m from
bmeurer@chromium.org, titzer@chromium.org, rossberg@chromium.org
Link to the patchset: https://codereview.chromium.org/1702063002/#ps60001
(title: "Fix a bug I introduced when rebasing")

[commit-bot: I haz the power, 2016-02-19 15:30:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1702063002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1702063002/60001

[commit-bot: I haz the power, 2016-02-19 15:59:03]

Description was changed from

==========
This patch implements an alternative approach to the rewriting
of non-pattern expressions, according to the (internally circulated)
design document.  Details to be provided here.

1.  RewritableAssignmentExpression has been renamed to RewritableExpression.
    It is a wrapper for AST nodes that wait for some potential rewriting
    (that may or may not happen).  Also, Is... and As... macros now see
    through RewritableExpressions.

2.  The function state keeps a list of rewritable expressions that must be
    rewritten only if they are used as non-pattern expressions.

3.  Expression classifiers are now templates, parameterized by parser
    traits.  They keep some additional state: a pointer to the list of
    non-pattern rewritable expressions.  It is important that expression
    classifiers be used strictly in a stack fashion, from now on.

4.  The RewriteNonPattern function has been simplified.

BUG=chromium:579913
LOG=N

Committed: https://crrev.com/7f5c864a6faf2b957b7273891e143b9bde35487c
Cr-Commit-Position: refs/heads/master@{#34154}
==========

to

==========
This patch implements an alternative approach to the rewriting
of non-pattern expressions, according to the (internally circulated)
design document.  Details to be provided here.

1.  RewritableAssignmentExpression has been renamed to RewritableExpression.
    It is a wrapper for AST nodes that wait for some potential rewriting
    (that may or may not happen).  Also, Is... and As... macros now see
    through RewritableExpressions.

2.  The function state keeps a list of rewritable expressions that must be
    rewritten only if they are used as non-pattern expressions.

3.  Expression classifiers are now templates, parameterized by parser
    traits.  They keep some additional state: a pointer to the list of
    non-pattern rewritable expressions.  It is important that expression
    classifiers be used strictly in a stack fashion, from now on.

4.  The RewriteNonPattern function has been simplified.

BUG=chromium:579913
LOG=N

Committed: https://crrev.com/7f5c864a6faf2b957b7273891e143b9bde35487c
Cr-Commit-Position: refs/heads/master@{#34154}
==========

[commit-bot: I haz the power, 2016-02-19 15:59:05]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-02-19 15:59:43]

Description was changed from

==========
This patch implements an alternative approach to the rewriting
of non-pattern expressions, according to the (internally circulated)
design document.  Details to be provided here.

1.  RewritableAssignmentExpression has been renamed to RewritableExpression.
    It is a wrapper for AST nodes that wait for some potential rewriting
    (that may or may not happen).  Also, Is... and As... macros now see
    through RewritableExpressions.

2.  The function state keeps a list of rewritable expressions that must be
    rewritten only if they are used as non-pattern expressions.

3.  Expression classifiers are now templates, parameterized by parser
    traits.  They keep some additional state: a pointer to the list of
    non-pattern rewritable expressions.  It is important that expression
    classifiers be used strictly in a stack fashion, from now on.

4.  The RewriteNonPattern function has been simplified.

BUG=chromium:579913
LOG=N

Committed: https://crrev.com/7f5c864a6faf2b957b7273891e143b9bde35487c
Cr-Commit-Position: refs/heads/master@{#34154}
==========

to

==========
This patch implements an alternative approach to the rewriting
of non-pattern expressions, according to the (internally circulated)
design document.  Details to be provided here.

1.  RewritableAssignmentExpression has been renamed to RewritableExpression.
    It is a wrapper for AST nodes that wait for some potential rewriting
    (that may or may not happen).  Also, Is... and As... macros now see
    through RewritableExpressions.

2.  The function state keeps a list of rewritable expressions that must be
    rewritten only if they are used as non-pattern expressions.

3.  Expression classifiers are now templates, parameterized by parser
    traits.  They keep some additional state: a pointer to the list of
    non-pattern rewritable expressions.  It is important that expression
    classifiers be used strictly in a stack fashion, from now on.

4.  The RewriteNonPattern function has been simplified.

BUG=chromium:579913
LOG=N

Committed: https://crrev.com/7f5c864a6faf2b957b7273891e143b9bde35487c
Cr-Commit-Position: refs/heads/master@{#34154}

Committed: https://crrev.com/ed665880416e5ece6561ab128b506896e2079809
Cr-Commit-Position: refs/heads/master@{#34162}
==========

[commit-bot: I haz the power, 2016-02-19 15:59:45]

Patchset 4 (id:??) landed as
https://crrev.com/ed665880416e5ece6561ab128b506896e2079809
Cr-Commit-Position: refs/heads/master@{#34162}