[parser] Refactor AstTraversalVisitor

[parser] Refactor AstTraversalVisitor

This patch parametrizes AstTraversalVisitor by the actual subclass,
in a similar way as AstVisitor is parametrized.  This allows a
subclass to, e.g., override the Visit method and still use the
traversal mechanism.  It also allows the subclass to override the
specific visiting methods, without them being virtual.

This patch also removes AstExpressionVisitor, subsuming its
functionality in AstTraversalVisitor.

R=adamk@chromium.org, verwaest@chromium.org
BUG=
LOG=N

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

[nickie, 2016-07-21 10:49:07]

The CQ bit was checked by nikolaos@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-21 10:49:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Yang, 2016-07-21 10:53:21]

yangguo@chromium.org changed reviewers:
+ yangguo@chromium.org

[Yang, 2016-07-21 10:53:21]

src/debug lgtm.

[Toon Verwaest, 2016-07-21 11:05:53]

lgtm

[commit-bot: I haz the power, 2016-07-21 11:11:19]

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

[commit-bot: I haz the power, 2016-07-21 11:11:20]

Dry run: This issue passed the CQ dry run.

[Toon Verwaest, 2016-07-21 11:16:59]

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

https://codereview.chromium.org/2169833002/diff/1/src/ast/ast.h#newcode2922
src/ast/ast.h:2922: return
this->impl()->Visit##NodeType(static_cast<NodeType*>(node));
This change isn't necessary if you add DEFINE_AST_VISITOR_SUBCLASS_MEMBERS() to
all leaf classes. That also means we don't need to make the private superclass
public in the Rewriter. (Granted, that's even a style-guide violation; I only
care because it may change semantics.)

I do like the idea of using this->impl() with the specific visit methods; but if
we decide to do this we should probably move this to AstVisitor<Subclass> in a
different CL, and remove it from these macros.

[nickie, 2016-07-21 14:15:24]

On 2016/07/21 11:16:59, Toon Verwaest wrote:
> https://codereview.chromium.org/2169833002/diff/1/src/ast/ast.h
> File src/ast/ast.h (right):
> 
> https://codereview.chromium.org/2169833002/diff/1/src/ast/ast.h#newcode2922
> src/ast/ast.h:2922: return
> this->impl()->Visit##NodeType(static_cast<NodeType*>(node));
> This change isn't necessary if you add DEFINE_AST_VISITOR_SUBCLASS_MEMBERS()
to
> all leaf classes. That also means we don't need to make the private superclass
> public in the Rewriter. (Granted, that's even a style-guide violation; I only
> care because it may change semantics.)
> 
> I do like the idea of using this->impl() with the specific visit methods; but
if
> we decide to do this we should probably move this to AstVisitor<Subclass> in a
> different CL, and remove it from these macros.

I made a second attempt, along Toon's suggestion, and the result is in
https://codereview.chromium.org/2166333002

It is broken, for reasons explained in that CL's description, and I don't think
it can be nicely fixed.  I tend to prefer this approach.

[adamk, 2016-07-21 18:11:57]

https://codereview.chromium.org/2169833002/diff/1/src/ast/ast-traversal-visit...
File src/ast/ast-traversal-visitor.h (right):

https://codereview.chromium.org/2169833002/diff/1/src/ast/ast-traversal-visit...
src/ast/ast-traversal-visitor.h:40: bool VisitExpression(Expression* node) {
return true; }
I don't see either of these methods overridden anywhere (VisitNode didn't exist
before, and this CL removes the only override of VisitExpression). Do we still
need this part of the design?

https://codereview.chromium.org/2169833002/diff/1/src/ast/ast-traversal-visit...
src/ast/ast-traversal-visitor.h:52: int depth() { return depth_; }
Nit: make this a const method

https://codereview.chromium.org/2169833002/diff/1/src/compiler.h
File src/compiler.h (right):

https://codereview.chromium.org/2169833002/diff/1/src/compiler.h#newcode9
src/compiler.h:9: #include "src/ast/ast-traversal-visitor.h"
I don't see AstTraversalVisitor used in this file, why is it included here?

https://codereview.chromium.org/2169833002/diff/1/src/parsing/parameter-initi...
File src/parsing/parameter-initializer-rewriter.cc (right):

https://codereview.chromium.org/2169833002/diff/1/src/parsing/parameter-initi...
src/parsing/parameter-initializer-rewriter.cc:30: protected:
"protected" + "final" doesn't make sense to me. Presumably these methods could
stay private; if you want to mark them in some way, a comment seems better than
this use of "protected"

https://codereview.chromium.org/2169833002/diff/1/src/parsing/parser.cc
File src/parsing/parser.cc (right):

https://codereview.chromium.org/2169833002/diff/1/src/parsing/parser.cc#newco...
src/parsing/parser.cc:4649: protected:
Same note about "protected", maybe I'm missing something?

https://codereview.chromium.org/2169833002/diff/1/src/v8.gyp
File src/v8.gyp (right):

https://codereview.chromium.org/2169833002/diff/1/src/v8.gyp#newcode455
src/v8.gyp:455: 'ast/ast-literal-reindexer.h',
Same here, add ast-traversal-visitor.h

[nickie, 2016-07-22 08:40:19]

https://codereview.chromium.org/2169833002/diff/1/BUILD.gn
File BUILD.gn (right):

https://codereview.chromium.org/2169833002/diff/1/BUILD.gn#newcode843
BUILD.gn:843: "src/ast/ast-numbering.h",
Adding a forgotten "src/ast/ast-traversal-visitor.h" here.

https://codereview.chromium.org/2169833002/diff/1/src/ast/ast-traversal-visit...
File src/ast/ast-traversal-visitor.h (right):

https://codereview.chromium.org/2169833002/diff/1/src/ast/ast-traversal-visit...
src/ast/ast-traversal-visitor.h:40: bool VisitExpression(Expression* node) {
return true; }
On 2016/07/21 18:11:57, adamk wrote:
> I don't see either of these methods overridden anywhere (VisitNode didn't
exist
> before, and this CL removes the only override of VisitExpression). Do we still
> need this part of the design? 

They're not yet.  The idea is to have them there, so that several visitors
(existing, or future) can be refactored using them, e.g. AstNumberingVisitor,
which pretty much does one thing all over the tree.  If the functionality is not
needed and these methods have dummy implementations, they are static and the
optimizer will make them disappear.

All that said, there are three ways to make specialized visitors:

1. Redefine (some or all) specific Visit* methods, to do something specialized
to some types of nodes (after dispatch).
2. Redefine VisitNode and/or VisitExpression, to do something regardless of the
type of node (before dispatch).
3. Redefine the Visit() method, to do something that requires absolute freedom.

Bottom line, if you don't want them, they're not needed.  But I suggest that we
keep them.

https://codereview.chromium.org/2169833002/diff/1/src/ast/ast-traversal-visit...
src/ast/ast-traversal-visitor.h:52: int depth() { return depth_; }
On 2016/07/21 18:11:57, adamk wrote:
> Nit: make this a const method

Done.

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

https://codereview.chromium.org/2169833002/diff/1/src/ast/ast.h#newcode2922
src/ast/ast.h:2922: return
this->impl()->Visit##NodeType(static_cast<NodeType*>(node));
On 2016/07/21 11:16:59, Toon Verwaest wrote:
> This change isn't necessary if you add DEFINE_AST_VISITOR_SUBCLASS_MEMBERS()
to
> all leaf classes. That also means we don't need to make the private superclass
> public in the Rewriter. (Granted, that's even a style-guide violation; I only
> care because it may change semantics.)
> 
> I do like the idea of using this->impl() with the specific visit methods; but
if
> we decide to do this we should probably move this to AstVisitor<Subclass> in a
> different CL, and remove it from these macros.

This was explored in https://codereview.chromium.org/2166333002/

https://codereview.chromium.org/2169833002/diff/1/src/compiler.h
File src/compiler.h (right):

https://codereview.chromium.org/2169833002/diff/1/src/compiler.h#newcode9
src/compiler.h:9: #include "src/ast/ast-traversal-visitor.h"
On 2016/07/21 18:11:57, adamk wrote:
> I don't see AstTraversalVisitor used in this file, why is it included here?

Right, it's not used here.  But the AstTraversalVisitor was defined in
src/ast/ast.h, which is included by this header file.  Therefore, if this line
is removed, then it has to be included in src/debug/liveedit.h.  As this file is
already in the patch, I'm making this change.

To make the picture complete, the decision to take the definition of
AstTraversalVisitor out of src/ast/ast.h was more or less a forced one.  When it
became a template, the implementation of its methods had to be in the header
file.  Some of them use the scope object, which is declared but not defined in
src/ast/ast.h.

https://codereview.chromium.org/2169833002/diff/1/src/parsing/parameter-initi...
File src/parsing/parameter-initializer-rewriter.cc (right):

https://codereview.chromium.org/2169833002/diff/1/src/parsing/parameter-initi...
src/parsing/parameter-initializer-rewriter.cc:30: protected:
On 2016/07/21 18:11:57, adamk wrote:
> "protected" + "final" doesn't make sense to me. Presumably these methods could
> stay private; if you want to mark them in some way, a comment seems better
than
> this use of "protected"

You're right.  I'm making them private again.  These are the only ones that the
friend class needs to be able to call and I'm adding a comment explaining that.

https://codereview.chromium.org/2169833002/diff/1/src/parsing/parser.cc
File src/parsing/parser.cc (right):

https://codereview.chromium.org/2169833002/diff/1/src/parsing/parser.cc#newco...
src/parsing/parser.cc:4649: protected:
On 2016/07/21 18:11:57, adamk wrote:
> Same note about "protected", maybe I'm missing something?

Nope, I was... :-)
Done, in the same way.

https://codereview.chromium.org/2169833002/diff/1/src/v8.gyp
File src/v8.gyp (right):

https://codereview.chromium.org/2169833002/diff/1/src/v8.gyp#newcode455
src/v8.gyp:455: 'ast/ast-literal-reindexer.h',
On 2016/07/21 18:11:57, adamk wrote:
> Same here, add ast-traversal-visitor.h

Done.

[adamk, 2016-07-22 18:02:31]

lgtm

https://codereview.chromium.org/2169833002/diff/1/src/ast/ast-traversal-visit...
File src/ast/ast-traversal-visitor.h (right):

https://codereview.chromium.org/2169833002/diff/1/src/ast/ast-traversal-visit...
src/ast/ast-traversal-visitor.h:40: bool VisitExpression(Expression* node) {
return true; }
On 2016/07/22 08:40:18, nickie wrote:
> On 2016/07/21 18:11:57, adamk wrote:
> > I don't see either of these methods overridden anywhere (VisitNode didn't
> exist
> > before, and this CL removes the only override of VisitExpression). Do we
still
> > need this part of the design? 
> 
> They're not yet.  The idea is to have them there, so that several visitors
> (existing, or future) can be refactored using them, e.g. AstNumberingVisitor,
> which pretty much does one thing all over the tree.  If the functionality is
not
> needed and these methods have dummy implementations, they are static and the
> optimizer will make them disappear.
> 
> All that said, there are three ways to make specialized visitors:
> 
> 1. Redefine (some or all) specific Visit* methods, to do something specialized
> to some types of nodes (after dispatch).
> 2. Redefine VisitNode and/or VisitExpression, to do something regardless of
the
> type of node (before dispatch).
> 3. Redefine the Visit() method, to do something that requires absolute
freedom.
> 
> Bottom line, if you don't want them, they're not needed.  But I suggest that
we
> keep them.

Makes sense, I wanted to be clear that they're not used yet. I'd recommend using
them soon so they get covered by tests, otherwise they might well rot away.

[nickie, 2016-07-25 07:32:22]

The CQ bit was checked by nikolaos@chromium.org

[nickie, 2016-07-25 07:32:23]

The patchset sent to the CQ was uploaded after l-g-t-m from
verwaest@chromium.org, yangguo@chromium.org
Link to the patchset: https://codereview.chromium.org/2169833002/#ps20001
(title: "Fixes according to reviewers' comments")

[commit-bot: I haz the power, 2016-07-25 07:32:33]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-25 07:33:42]

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

[commit-bot: I haz the power, 2016-07-25 07:33:44]

Try jobs failed on following builders:
  v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
  v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/9592)
  v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...)
  v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/9573)
  v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...)
  v8_linux_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)
  v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/5555)
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/20094)

[nickie, 2016-07-25 08:00:56]

The CQ bit was checked by nikolaos@chromium.org

[nickie, 2016-07-25 08:00:56]

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

[commit-bot: I haz the power, 2016-07-25 08:01:04]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-25 08:25:54]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-07-25 08:35:22]

Description was changed from

==========
[parser] Refactor AstTraversalVisitor

This patch parametrizes AstTraversalVisitor by the actual subclass,
in a similar way as AstVisitor is parametrized.  This allows a
subclass to, e.g., override the Visit method and still use the
traversal mechanism.  It also allows the subclass to override the
specific visiting methods, without them being virtual.

This patch also removes AstExpressionVisitor, subsuming its
functionality in AstTraversalVisitor.

R=adamk@chromium.org, verwaest@chromium.org
BUG=
LOG=N
==========

to

==========
[parser] Refactor AstTraversalVisitor

This patch parametrizes AstTraversalVisitor by the actual subclass,
in a similar way as AstVisitor is parametrized.  This allows a
subclass to, e.g., override the Visit method and still use the
traversal mechanism.  It also allows the subclass to override the
specific visiting methods, without them being virtual.

This patch also removes AstExpressionVisitor, subsuming its
functionality in AstTraversalVisitor.

R=adamk@chromium.org, verwaest@chromium.org
BUG=
LOG=N

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

[commit-bot: I haz the power, 2016-07-25 08:35:23]

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