Templatize AstVisitor with its subclass

Templatize AstVisitor with its subclass

This replaces the vtable on AstNode with a NodeType tag. The visitors replace double dispatch with a single switch over the NodeType.

For now, visitors with subclasses still have virtual methods themselves. We should probably specialize them later as well.

The uint8_t NodeType allows us to better pack memory, saving 8-16 bytes on many AST nodes (with additional packing that I'll do in a follow-up CL)

BUG=

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

[Toon Verwaest, 2016-07-13 16:40:00]

verwaest@chromium.org changed reviewers:
+ ishell@chromium.org, mstarzinger@chromium.org, vogelheim@chromium.org

[Toon Verwaest, 2016-07-13 16:40:00]

ptal. Depends on https://codereview.chromium.org/2126233002/
vogelheim: ptal at parser

[Toon Verwaest, 2016-07-13 16:42:03]

Description was changed from

==========
Templatize AstVisitor with its subclass

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

to

==========
Templatize AstVisitor with its subclass

This replaces the vtable on AstNode with a NodeType tag. The visitors replace
double dispatch with a single switch over the NodeType.

For now, visitors with subclasses still have virtual methods themselves. We
should probably specialize them later as well.

The uint8_t NodeType allows us to better pack memory, saving 8-16 bytes on many
AST nodes (with additional packing that I'll do in a follow-up CL)

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

[Toon Verwaest, 2016-07-13 16:43:37]

Patchset #2 (id:20001) has been deleted

[Toon Verwaest, 2016-07-13 16:43:43]

Patchset #1 (id:1) has been deleted

[vogelheim, 2016-07-14 11:27:35]

lgtm (for src/parsing/*, and non-OWNER-lgtm for anything else, modulo some
nitpicks.)

https://codereview.chromium.org/2142233003/diff/40001/src/ast/ast.h
File src/ast/ast.h (right):

https://codereview.chromium.org/2142233003/diff/40001/src/ast/ast.h#newcode190
src/ast/ast.h:190: enum NodeType : uint8_t { kModule = 0,
AST_NODE_LIST(DECLARE_TYPE_ENUM) };
This is super weird: If Module is an AST node, shouldn't it really be in
AST_NODE_LIST? If other uses of AST_NODE_LIST aren't supposed to "see" Module
nodes, maybe keep AST_NODE_LIST but have another macro for the list of
really-truly-all-AstNode-instances?

[Admittedly this issue didn't originate in your CL. So feel free to ignore.]

https://codereview.chromium.org/2142233003/diff/40001/src/ast/ast.h#newcode195
src/ast/ast.h:195: explicit AstNode(int position, NodeType type)
nitpick: drop explicit (not needed w/ 2 args)

[Here & additional instances below]

https://codereview.chromium.org/2142233003/diff/40001/src/ast/ast.h#newcode195
src/ast/ast.h:195: explicit AstNode(int position, NodeType type)
more nitpick: This used to be an abstract class, so you couldn't (inadvertently)
instantiate it. Maybe make the constructor protected, to emulate the this?

[Here, and other formerly uninstantiable classes.]

https://codereview.chromium.org/2142233003/diff/40001/src/ast/ast.h#newcode2889
src/ast/ast.h:2889: // - leaf node visitors are abstract.
nitpick: comment is untrue now, is it? 

It might be good to have a 1-line comment on how this is intended to be used,
e.g.:
// Sub-class should parameterize AstVisitor with itself. E.g.:
//   class SpecificVisitor : public AstVisitor<SpecificVisitor> { .. }

https://codereview.chromium.org/2142233003/diff/40001/src/ast/ast.h#newcode2896
src/ast/ast.h:2896: // Iteration left-to-right.
nitpick: In the previous version, I read the comment as applying to all 3
following Visit*(..) methods. Here, it looks like it only applied to
VisitDeclarations, which - while technically correct - doesn't make that much
sense.

https://codereview.chromium.org/2142233003/diff/40001/src/ast/ast.h#newcode2937
src/ast/ast.h:2937: #define DEFINE_AST_VISITOR_MEMBERS_WITHOUT_STACKOVERFLOW()  
 \
Super nitpick:
- I was wondering about the name of the macro, and it really only makes any
sense if one has read the DEFINE_AST_VISITOR_SUBCLASS_MEMBERS one below...
- Maybe WITHOUT_STACKOVERFLOW_CHECK? Or just move it below the other definition?

[Michael Starzinger, 2016-07-14 13:02:45]

LGTM from my end. Modulo Daniel's comments, which all make sense. I looked at
everything outside of "parsing".

[Igor Sheludko, 2016-07-14 16:39:54]

lgtm modulo nits:

https://codereview.chromium.org/2142233003/diff/40001/src/ast/ast-expression-...
File src/ast/ast-expression-rewriter.h (right):

https://codereview.chromium.org/2142233003/diff/40001/src/ast/ast-expression-...
src/ast/ast-expression-rewriter.h:20: class AstExpressionRewriter : public
AstVisitor<AstExpressionRewriter> {
Same here.

https://codereview.chromium.org/2142233003/diff/40001/src/ast/ast.cc
File src/ast/ast.cc (right):

https://codereview.chromium.org/2142233003/diff/40001/src/ast/ast.cc#newcode41
src/ast/ast.cc:41: #define RETURN_NODE(Node) \
This and below changes should have already been included into previous CL, I
suppose.

https://codereview.chromium.org/2142233003/diff/40001/src/ast/ast.h
File src/ast/ast.h (right):

https://codereview.chromium.org/2142233003/diff/40001/src/ast/ast.h#newcode130
src/ast/ast.h:130: #define DECLARE_NODE_TYPE(type)                              
           \
As an idea for a next CL: This macro does not make sense anymore.

https://codereview.chromium.org/2142233003/diff/40001/src/ast/prettyprinter.h
File src/ast/prettyprinter.h (right):

https://codereview.chromium.org/2142233003/diff/40001/src/ast/prettyprinter.h...
src/ast/prettyprinter.h:56: class PrettyPrinter : public
AstVisitor<PrettyPrinter> {
Same here (see HOptimizedGraphBuilder).

https://codereview.chromium.org/2142233003/diff/40001/src/compiler/ast-graph-...
File src/compiler/ast-graph-builder.h (right):

https://codereview.chromium.org/2142233003/diff/40001/src/compiler/ast-graph-...
src/compiler/ast-graph-builder.h:35: class AstGraphBuilder : public
AstVisitor<AstGraphBuilder> {
Same comment as in HOptimizedGraphBuilder is requested.

https://codereview.chromium.org/2142233003/diff/40001/src/crankshaft/hydrogen.h
File src/crankshaft/hydrogen.h (right):

https://codereview.chromium.org/2142233003/diff/40001/src/crankshaft/hydrogen...
src/crankshaft/hydrogen.h:2065: class HOptimizedGraphBuilder : public
HGraphBuilder,
Please add a comment that although the HOptimizedGraphBuilder class is not final
the visiting will still work for HOptimizedGraphBuilderWithPositions subclass
because this class defines VisitXXX() methods as virtual.

[Toon Verwaest, 2016-07-15 07:07:18]

verwaest@chromium.org changed reviewers:
+ ahaas@chromium.org

[Toon Verwaest, 2016-07-15 07:07:20]

Addressed comments.
@ahaas: ptal at wasm-related changes.

https://codereview.chromium.org/2142233003/diff/40001/src/ast/ast-expression-...
File src/ast/ast-expression-rewriter.h (right):

https://codereview.chromium.org/2142233003/diff/40001/src/ast/ast-expression-...
src/ast/ast-expression-rewriter.h:20: class AstExpressionRewriter : public
AstVisitor<AstExpressionRewriter> {
On 2016/07/14 16:39:54, Igor Sheludko wrote:
> Same here.

Done.

https://codereview.chromium.org/2142233003/diff/40001/src/ast/ast.cc
File src/ast/ast.cc (right):

https://codereview.chromium.org/2142233003/diff/40001/src/ast/ast.cc#newcode41
src/ast/ast.cc:41: #define RETURN_NODE(Node) \
On 2016/07/14 16:39:54, Igor Sheludko wrote:
> This and below changes should have already been included into previous CL, I
> suppose.

Acknowledged.

https://codereview.chromium.org/2142233003/diff/40001/src/ast/ast.h
File src/ast/ast.h (right):

https://codereview.chromium.org/2142233003/diff/40001/src/ast/ast.h#newcode130
src/ast/ast.h:130: #define DECLARE_NODE_TYPE(type)                              
           \
On 2016/07/14 16:39:54, Igor Sheludko wrote:
> As an idea for a next CL: This macro does not make sense anymore.

Acknowledged.

https://codereview.chromium.org/2142233003/diff/40001/src/ast/ast.h#newcode190
src/ast/ast.h:190: enum NodeType : uint8_t { kModule = 0,
AST_NODE_LIST(DECLARE_TYPE_ENUM) };
On 2016/07/14 11:27:35, vogelheim wrote:
> This is super weird: If Module is an AST node, shouldn't it really be in
> AST_NODE_LIST? If other uses of AST_NODE_LIST aren't supposed to "see" Module
> nodes, maybe keep AST_NODE_LIST but have another macro for the list of
> really-truly-all-AstNode-instances?
> 
> [Admittedly this issue didn't originate in your CL. So feel free to ignore.]

I agree, but it's a little involved. This should be done in a different CL I
think.

https://codereview.chromium.org/2142233003/diff/40001/src/ast/ast.h#newcode195
src/ast/ast.h:195: explicit AstNode(int position, NodeType type)
On 2016/07/14 11:27:35, vogelheim wrote:
> nitpick: drop explicit (not needed w/ 2 args)
> 
> [Here & additional instances below]

Done.

https://codereview.chromium.org/2142233003/diff/40001/src/ast/ast.h#newcode2889
src/ast/ast.h:2889: // - leaf node visitors are abstract.
On 2016/07/14 11:27:35, vogelheim wrote:
> nitpick: comment is untrue now, is it? 
> 
> It might be good to have a 1-line comment on how this is intended to be used,
> e.g.:
> // Sub-class should parameterize AstVisitor with itself. E.g.:
> //   class SpecificVisitor : public AstVisitor<SpecificVisitor> { .. }

Done.

https://codereview.chromium.org/2142233003/diff/40001/src/ast/ast.h#newcode2896
src/ast/ast.h:2896: // Iteration left-to-right.
On 2016/07/14 11:27:35, vogelheim wrote:
> nitpick: In the previous version, I read the comment as applying to all 3
> following Visit*(..) methods. Here, it looks like it only applied to
> VisitDeclarations, which - while technically correct - doesn't make that much
> sense.

Done.

https://codereview.chromium.org/2142233003/diff/40001/src/ast/ast.h#newcode2937
src/ast/ast.h:2937: #define DEFINE_AST_VISITOR_MEMBERS_WITHOUT_STACKOVERFLOW()  
 \
On 2016/07/14 11:27:35, vogelheim wrote:
> Super nitpick:
> - I was wondering about the name of the macro, and it really only makes any
> sense if one has read the DEFINE_AST_VISITOR_SUBCLASS_MEMBERS one below...
> - Maybe WITHOUT_STACKOVERFLOW_CHECK? Or just move it below the other
definition?

Done. This macro should probably go away. We could probably pass in the stack
limit directly rather than the isolate. I think we already do this somewhere.
Different CL though.

https://codereview.chromium.org/2142233003/diff/40001/src/ast/prettyprinter.h
File src/ast/prettyprinter.h (right):

https://codereview.chromium.org/2142233003/diff/40001/src/ast/prettyprinter.h...
src/ast/prettyprinter.h:56: class PrettyPrinter : public
AstVisitor<PrettyPrinter> {
On 2016/07/14 16:39:54, Igor Sheludko wrote:
> Same here (see HOptimizedGraphBuilder).

Done.

https://codereview.chromium.org/2142233003/diff/40001/src/compiler/ast-graph-...
File src/compiler/ast-graph-builder.h (right):

https://codereview.chromium.org/2142233003/diff/40001/src/compiler/ast-graph-...
src/compiler/ast-graph-builder.h:35: class AstGraphBuilder : public
AstVisitor<AstGraphBuilder> {
On 2016/07/14 16:39:54, Igor Sheludko wrote:
> Same comment as in HOptimizedGraphBuilder is requested.

Done.

https://codereview.chromium.org/2142233003/diff/40001/src/crankshaft/hydrogen.h
File src/crankshaft/hydrogen.h (right):

https://codereview.chromium.org/2142233003/diff/40001/src/crankshaft/hydrogen...
src/crankshaft/hydrogen.h:2065: class HOptimizedGraphBuilder : public
HGraphBuilder,
On 2016/07/14 16:39:54, Igor Sheludko wrote:
> Please add a comment that although the HOptimizedGraphBuilder class is not
final
> the visiting will still work for HOptimizedGraphBuilderWithPositions subclass
> because this class defines VisitXXX() methods as virtual.

Done.

[Igor Sheludko, 2016-07-15 07:12:11]

lgtm

[ahaas, 2016-07-15 07:28:10]

On 2016/07/15 at 07:12:11, ishell wrote:
> lgtm

wasm lgtm

[Toon Verwaest, 2016-07-15 07:31:34]

The CQ bit was checked by verwaest@chromium.org

[Toon Verwaest, 2016-07-15 07:31:35]

The patchset sent to the CQ was uploaded after l-g-t-m from
mstarzinger@chromium.org, vogelheim@chromium.org
Link to the patchset: https://codereview.chromium.org/2142233003/#ps80001
(title: "Addressed comments")

[commit-bot: I haz the power, 2016-07-15 07:31:48]

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-15 07:56:14]

Description was changed from

==========
Templatize AstVisitor with its subclass

This replaces the vtable on AstNode with a NodeType tag. The visitors replace
double dispatch with a single switch over the NodeType.

For now, visitors with subclasses still have virtual methods themselves. We
should probably specialize them later as well.

The uint8_t NodeType allows us to better pack memory, saving 8-16 bytes on many
AST nodes (with additional packing that I'll do in a follow-up CL)

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

to

==========
Templatize AstVisitor with its subclass

This replaces the vtable on AstNode with a NodeType tag. The visitors replace
double dispatch with a single switch over the NodeType.

For now, visitors with subclasses still have virtual methods themselves. We
should probably specialize them later as well.

The uint8_t NodeType allows us to better pack memory, saving 8-16 bytes on many
AST nodes (with additional packing that I'll do in a follow-up CL)

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

[commit-bot: I haz the power, 2016-07-15 07:56:16]

Committed patchset #3 (id:80001)

[commit-bot: I haz the power, 2016-07-15 07:58:18]

Description was changed from

==========
Templatize AstVisitor with its subclass

This replaces the vtable on AstNode with a NodeType tag. The visitors replace
double dispatch with a single switch over the NodeType.

For now, visitors with subclasses still have virtual methods themselves. We
should probably specialize them later as well.

The uint8_t NodeType allows us to better pack memory, saving 8-16 bytes on many
AST nodes (with additional packing that I'll do in a follow-up CL)

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

to

==========
Templatize AstVisitor with its subclass

This replaces the vtable on AstNode with a NodeType tag. The visitors replace
double dispatch with a single switch over the NodeType.

For now, visitors with subclasses still have virtual methods themselves. We
should probably specialize them later as well.

The uint8_t NodeType allows us to better pack memory, saving 8-16 bytes on many
AST nodes (with additional packing that I'll do in a follow-up CL)

BUG=

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

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

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