Add Kernel AST nodes for Vector and Closure primitives in VM

Add Kernel AST nodes for Vector and Closure primitives in VM

Five Kernel AST nodes added: VectorCreation, VectorGet, VectorSet,
VectorCopy, and ClosureCreation.

R=asgerf@google.com, kmillikin@google.com

Committed: https://github.com/dart-lang/sdk/commit/95560cb02a789e1c2e8bb4c2efd84990a3a4b9cb

[Dmitry Stefantsov, 2017-04-04 10:47:09]

Description was changed from

==========
Add Kernel AST nodes for Vector and Closure primitives

Five Kernel AST nodes added: VectorCreation, VectorGet, VectorSet,
VectorCopy, and ClosureCreation.
==========

to

==========
Add Kernel AST nodes for Vector and Closure primitives in VM

Five Kernel AST nodes added: VectorCreation, VectorGet, VectorSet,
VectorCopy, and ClosureCreation.
==========

[Dmitry Stefantsov, 2017-04-04 10:47:59]

dmitryas@google.com changed reviewers:
+ asgerf@google.com, karlklose@google.com

[asgerf, 2017-04-04 10:53:28]

LGTM but please also get a look from Kevin or Martin

[Dmitry Stefantsov, 2017-04-04 10:59:01]

dmitryas@google.com changed reviewers:
+ kmillikin@google.com

[Kevin Millikin (Google), 2017-04-04 11:29:34]

Comments below.

https://codereview.chromium.org/2792333002/diff/1/runtime/vm/kernel.h
File runtime/vm/kernel.h (right):

https://codereview.chromium.org/2792333002/diff/1/runtime/vm/kernel.h#newcode...
runtime/vm/kernel.h:1957: int64_t value_;
Use intptr_t, here and elsewhere.

https://codereview.chromium.org/2792333002/diff/1/runtime/vm/kernel.h#newcode...
runtime/vm/kernel.h:1958: TokenPosition position_;
This does not need a token position or an end token position, does it?

https://codereview.chromium.org/2792333002/diff/1/runtime/vm/kernel.h#newcode...
runtime/vm/kernel.h:1986: Child<Expression> vector_expression_;
Is it the case that this is always a VariableGet, because contexts are always
named?  We might want to specialize for that case by changing this to
Ref<VariableDeclaration> and similarly below.

https://codereview.chromium.org/2792333002/diff/1/runtime/vm/kernel.h#newcode...
runtime/vm/kernel.h:1987: int64_t index_;
intptr_t

https://codereview.chromium.org/2792333002/diff/1/runtime/vm/kernel.h#newcode...
runtime/vm/kernel.h:1988: TokenPosition position_;
We don't need these token positions.

https://codereview.chromium.org/2792333002/diff/1/runtime/vm/kernel.h#newcode...
runtime/vm/kernel.h:2018: int64_t index_;
intptr_t

https://codereview.chromium.org/2792333002/diff/1/runtime/vm/kernel.h#newcode...
runtime/vm/kernel.h:2020: TokenPosition position_;
I guess this will need a position and possibly two, but they have to come from
the binary file.  I'd just leave them out for now since we don't know exactly
what they'll be.

https://codereview.chromium.org/2792333002/diff/1/runtime/vm/kernel.h#newcode...
runtime/vm/kernel.h:2048: TokenPosition position_;
We shouldn't need these positions.

https://codereview.chromium.org/2792333002/diff/1/runtime/vm/kernel.h#newcode...
runtime/vm/kernel.h:2066: CanonicalName* top_level_function_reference() {
For consistency, we don't usually put _reference in the name of this getter.

I don't think it's important that it's a top level function, it seems sort of
incidental.  I'd probably choose a name like closed_function() or
static_function().

https://codereview.chromium.org/2792333002/diff/1/runtime/vm/kernel.h#newcode...
runtime/vm/kernel.h:2082: TokenPosition position_;
I don't think we need these positions.

[Dmitry Stefantsov, 2017-04-04 12:51:14]

PTAL

https://codereview.chromium.org/2792333002/diff/1/runtime/vm/kernel.h
File runtime/vm/kernel.h (right):

https://codereview.chromium.org/2792333002/diff/1/runtime/vm/kernel.h#newcode...
runtime/vm/kernel.h:1957: int64_t value_;
On 2017/04/04 11:29:33, Kevin Millikin (Google) wrote:
> Use intptr_t, here and elsewhere.

Done.

https://codereview.chromium.org/2792333002/diff/1/runtime/vm/kernel.h#newcode...
runtime/vm/kernel.h:1958: TokenPosition position_;
On 2017/04/04 11:29:34, Kevin Millikin (Google) wrote:
> This does not need a token position or an end token position, does it?

No, I guess not. Thanks! Done.

https://codereview.chromium.org/2792333002/diff/1/runtime/vm/kernel.h#newcode...
runtime/vm/kernel.h:1986: Child<Expression> vector_expression_;
On 2017/04/04 11:29:33, Kevin Millikin (Google) wrote:
> Is it the case that this is always a VariableGet, because contexts are always
> named?  We might want to specialize for that case by changing this to
> Ref<VariableDeclaration> and similarly below.

Actually, it's not always VariableGet. In case of a parent context it's
retrieved from 0th position of its child context, so 'context[0]' may also be a
vector expression.

https://codereview.chromium.org/2792333002/diff/1/runtime/vm/kernel.h#newcode...
runtime/vm/kernel.h:1987: int64_t index_;
On 2017/04/04 11:29:34, Kevin Millikin (Google) wrote:
> intptr_t

Done.

https://codereview.chromium.org/2792333002/diff/1/runtime/vm/kernel.h#newcode...
runtime/vm/kernel.h:1988: TokenPosition position_;
On 2017/04/04 11:29:34, Kevin Millikin (Google) wrote:
> We don't need these token positions.

I guess we may want these, because they replace variable reads. But I guess it's
ok to remove them now.

https://codereview.chromium.org/2792333002/diff/1/runtime/vm/kernel.h#newcode...
runtime/vm/kernel.h:2018: int64_t index_;
On 2017/04/04 11:29:33, Kevin Millikin (Google) wrote:
> intptr_t

Done.

https://codereview.chromium.org/2792333002/diff/1/runtime/vm/kernel.h#newcode...
runtime/vm/kernel.h:2020: TokenPosition position_;
On 2017/04/04 11:29:34, Kevin Millikin (Google) wrote:
> I guess this will need a position and possibly two, but they have to come from
> the binary file.  I'd just leave them out for now since we don't know exactly
> what they'll be.

Agree. Done.

https://codereview.chromium.org/2792333002/diff/1/runtime/vm/kernel.h#newcode...
runtime/vm/kernel.h:2048: TokenPosition position_;
On 2017/04/04 11:29:34, Kevin Millikin (Google) wrote:
> We shouldn't need these positions.

Agree. Done.

https://codereview.chromium.org/2792333002/diff/1/runtime/vm/kernel.h#newcode...
runtime/vm/kernel.h:2066: CanonicalName* top_level_function_reference() {
On 2017/04/04 11:29:33, Kevin Millikin (Google) wrote:
> For consistency, we don't usually put _reference in the name of this getter.
> 
> I don't think it's important that it's a top level function, it seems sort of
> incidental.  I'd probably choose a name like closed_function() or
> static_function().

Somehow I've missed this convention. Thanks! Fixed.

https://codereview.chromium.org/2792333002/diff/1/runtime/vm/kernel.h#newcode...
runtime/vm/kernel.h:2082: TokenPosition position_;
On 2017/04/04 11:29:34, Kevin Millikin (Google) wrote:
> I don't think we need these positions.

Done.

[Kevin Millikin (Google), 2017-04-05 08:34:27]

LGTM with two changes.

https://codereview.chromium.org/2792333002/diff/20001/runtime/vm/kernel_binar...
File runtime/vm/kernel_binary.cc (right):

https://codereview.chromium.org/2792333002/diff/20001/runtime/vm/kernel_binar...
runtime/vm/kernel_binary.cc:972: PositionScope scope(reader);
We don't this scoped helper object here, or below.

https://codereview.chromium.org/2792333002/diff/20001/runtime/vm/kernel_binar...
runtime/vm/kernel_binary.cc:977: vector_creation->value_ =
static_cast<intptr_t>(reader->ReadUInt());
We don't need a static_cast here, or below, because there is an implicit
conversion from uint32_t to intptr_t.  The conversion won't change the value
because the uint32_t is a 30-bit unsigned integer.

[Dmitry Stefantsov, 2017-04-05 13:59:10]

https://codereview.chromium.org/2792333002/diff/20001/runtime/vm/kernel_binar...
File runtime/vm/kernel_binary.cc (right):

https://codereview.chromium.org/2792333002/diff/20001/runtime/vm/kernel_binar...
runtime/vm/kernel_binary.cc:972: PositionScope scope(reader);
On 2017/04/05 08:34:27, Kevin Millikin (Google) wrote:
> We don't this scoped helper object here, or below.

Done.

https://codereview.chromium.org/2792333002/diff/20001/runtime/vm/kernel_binar...
runtime/vm/kernel_binary.cc:977: vector_creation->value_ =
static_cast<intptr_t>(reader->ReadUInt());
On 2017/04/05 08:34:27, Kevin Millikin (Google) wrote:
> We don't need a static_cast here, or below, because there is an implicit
> conversion from uint32_t to intptr_t.  The conversion won't change the value
> because the uint32_t is a 30-bit unsigned integer.

Got it. Thanks! Fixed.

[Dmitry Stefantsov, 2017-04-05 13:59:37]

Description was changed from

==========
Add Kernel AST nodes for Vector and Closure primitives in VM

Five Kernel AST nodes added: VectorCreation, VectorGet, VectorSet,
VectorCopy, and ClosureCreation.
==========

to

==========
Add Kernel AST nodes for Vector and Closure primitives in VM

Five Kernel AST nodes added: VectorCreation, VectorGet, VectorSet,
VectorCopy, and ClosureCreation.

R=asgerf@google.com, kmillikin@google.com

Committed:
https://github.com/dart-lang/sdk/commit/95560cb02a789e1c2e8bb4c2efd84990a3a4b9cb
==========

[Dmitry Stefantsov, 2017-04-05 13:59:38]

Committed patchset #3 (id:40001) manually as
95560cb02a789e1c2e8bb4c2efd84990a3a4b9cb (presubmit successful).