Add a generic mechanism for expression rewriting

Add a generic mechanism for expression rewriting

R=rossberg@chromium.org
BUG=

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

[rossberg, 2016-01-08 10:36:57]

Looks good, just a question regarding the REWRITE macro.

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

https://codereview.chromium.org/1565153002/diff/1/src/ast/ast.h#newcode3415
src/ast/ast.h:3415: #define AST_REWRITE(Type, GET, SET)                         
  \
Can this macro be simplified to just AST_REWRITE(Type, name),

https://codereview.chromium.org/1565153002/diff/1/src/ast/ast.h#newcode3415
src/ast/ast.h:3415: #define AST_REWRITE(Type, GET, SET)                         
  \
Is there a strong reason to put this here instead of ast-rewriter.cc?

Also, do you think it's worth adding a second wrapper macro around this one,
like AST_REWRITE_PROPERTY(Type, name)? Almost all invocations are of that form.

[nickie, 2016-01-08 13:22:09]

I'd leave it as it is, but I have no objection with adding a
AST_REWRITE_PROPERTY macro.

I think that moving the macros to the cc file will make it really hard to write
non-trivial classes derived from AstExpressionRewriter.

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

https://codereview.chromium.org/1565153002/diff/1/src/ast/ast.h#newcode3415
src/ast/ast.h:3415: #define AST_REWRITE(Type, GET, SET)                         
  \
On 2016/01/08 10:36:57, rossberg wrote:
> Can this macro be simplified to just AST_REWRITE(Type, name),

Most of its uses are of the form:

    AST_REWRITE(Statement, node->body(), node->set_body(replacement));

In these cases, we could assume that there are getter/setter methods called
"##name()" and "set_# ##name(Type)", but still the "node" object would either
have to be hardwired, or made into a third parameter.

However, there are some uses of the form:

    AST_REWRITE(Statement, statements->at(i), statements->Set(i, replacement));

and these are the main reason why I did not try to make something fancier here. 
It's not clear what these will become.  Unless you want to have two macros --- a
simpler and a more generic.

https://codereview.chromium.org/1565153002/diff/1/src/ast/ast.h#newcode3415
src/ast/ast.h:3415: #define AST_REWRITE(Type, GET, SET)                         
  \
On 2016/01/08 10:36:57, rossberg wrote:
> Is there a strong reason to put this here instead of ast-rewriter.cc?

I think that the macro will be useful in classes derived from
AstExpressionRewriter, to define specific rewriting behaviour.  That's why I put
it here.  So far, it's only used from ast-expression-rewriter.cc.

> Also, do you think it's worth adding a second wrapper macro around this one,
> like AST_REWRITE_PROPERTY(Type, name)? Almost all invocations are of that
form.

Yes, this was addressed in the comment above.  I think it would need a third
parameter (the object whose property we're replacing).

[rossberg, 2016-01-08 15:07:13]

Okay, leaving it up to you.

LGTM

[nickie, 2016-01-11 10:24:18]

https://codereview.chromium.org/1565153002/diff/20001/src/ast/ast.h
File src/ast/ast.h (right):

https://codereview.chromium.org/1565153002/diff/20001/src/ast/ast.h#newcode930
src/ast/ast.h:930: void set_assign_each(Expression* e) { assign_each_ = e; }
Small fix #1: these four lines had inadvertently been committed with a later CL.

https://codereview.chromium.org/1565153002/diff/20001/src/ast/ast.h#newcode3395
src/ast/ast.h:3395: if (HasStackOverflow()) return node;              \
Small fix #2: in case of failure, a non-null node is returned.

[rossberg, 2016-01-11 12:09:34]

lgtm

[nickie, 2016-01-11 12:18:56]

The CQ bit was checked by nikolaos@chromium.org

[commit-bot: I haz the power, 2016-01-11 12:19:02]

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

[commit-bot: I haz the power, 2016-01-11 12:41:50]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-01-11 12:42:16]

Description was changed from

==========
Add a generic mechanism for expression rewriting

R=rossberg@chromium.org
BUG=
==========

to

==========
Add a generic mechanism for expression rewriting

R=rossberg@chromium.org
BUG=

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

[commit-bot: I haz the power, 2016-01-11 12:42:17]

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