Setup type and modifiers of local variables on creation.

Setup type and modifiers of local variables on creation.

R=karlklose@google.com, paulberry@google.com

Committed: https://github.com/dart-lang/sdk/commit/7e8639eebf77ae073805e42bf617ba73cd09ee4a

[ahe, 2017-03-23 09:41:27]

ahe@google.com changed reviewers:
+ karlklose@google.com, paulberry@google.com

[ahe, 2017-03-23 09:41:28]

I only needed modifiers, but I threw types in there as I hope this will help
inference. I think the end result is much cleaner.

[karlklose, 2017-03-23 09:52:15]

LGTM

https://codereview.chromium.org/2769113004/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/kernel/body_builder.dart (right):

https://codereview.chromium.org/2769113004/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:919: bool isConst =
(currentLocalVariableModifiers & constMask) != 0;
assert(currentLocalVariableModifiers != -1) here?

[Paul Berry, 2017-03-23 10:50:52]

lgtm

https://codereview.chromium.org/2769113004/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/kernel/body_builder.dart (right):

https://codereview.chromium.org/2769113004/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:958:
super.push(currentLocalVariableModifiers);
Why not:

push(currentLocalVariableModifiers ?? NullValue.LocalVariableModifiers);

(and add `LocalVariableModifiers` to the `NullValue` enum)

https://codereview.chromium.org/2769113004/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:959:
super.push(currentLocalVariableType);
Similar question here.

https://codereview.chromium.org/2769113004/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:960:
currentLocalVariableType = type ?? const DynamicType();
FYI, for type inference we will need to keep track of whether an explicit type
was provided, so consider moving the `?? const DynamicType()` to line 924.

https://codereview.chromium.org/2769113004/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:962:
super.push(constantExpressionRequired);
AFAICT, `constantExpressionRequired` is never null, so I think we can do:

push(constantExpressionRequired)

[ahe, 2017-03-23 11:17:55]

Thank you, Paul and Karl!

https://codereview.chromium.org/2769113004/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/kernel/body_builder.dart (right):

https://codereview.chromium.org/2769113004/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:919: bool isConst =
(currentLocalVariableModifiers & constMask) != 0;
On 2017/03/23 09:52:15, karlklose wrote:
> assert(currentLocalVariableModifiers != -1) here?

Done.

https://codereview.chromium.org/2769113004/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:958:
super.push(currentLocalVariableModifiers);
On 2017/03/23 10:50:52, Paul Berry wrote:
> Why not:
> 
> push(currentLocalVariableModifiers ?? NullValue.LocalVariableModifiers);
> 
> (and add `LocalVariableModifiers` to the `NullValue` enum)

I need to use super.push to avoid changing inInitializer. I'll look into
cleaning that up.

Based on some old advice regarding performance, I'm avoiding initializing an int
field to null, hence currentLocalVariableModifiers can't be null.

https://codereview.chromium.org/2769113004/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:959:
super.push(currentLocalVariableType);
On 2017/03/23 10:50:52, Paul Berry wrote:
> Similar question here.

I'll change it to ?? NullValue.Type.

https://codereview.chromium.org/2769113004/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:960:
currentLocalVariableType = type ?? const DynamicType();
On 2017/03/23 10:50:52, Paul Berry wrote:
> FYI, for type inference we will need to keep track of whether an explicit type
> was provided, so consider moving the `?? const DynamicType()` to line 924.

Done.

https://codereview.chromium.org/2769113004/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:962:
super.push(constantExpressionRequired);
On 2017/03/23 10:50:52, Paul Berry wrote:
> AFAICT, `constantExpressionRequired` is never null, so I think we can do:
> 
> push(constantExpressionRequired)

As mentioned above, super.push is used to avoid interfering with inInitializer.

[ahe, 2017-03-23 11:19:43]

Description was changed from

==========
Setup type and modifiers of local variables on creation.
==========

to

==========
Setup type and modifiers of local variables on creation.

R=karlklose@google.com, paulberry@google.com

Committed:
https://github.com/dart-lang/sdk/commit/7e8639eebf77ae073805e42bf617ba73cd09ee4a
==========

[ahe, 2017-03-23 11:19:44]

Committed patchset #2 (id:20001) manually as
7e8639eebf77ae073805e42bf617ba73cd09ee4a (presubmit successful).