VM: [Kernel] Partial support for checked mode.

VM: [Kernel] Partial support for checked mode.

R=vegorov@google.com

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

[kustermann, 2017-01-30 17:34:07]

kustermann@google.com changed reviewers:
+ vegorov@google.com

[Vyacheslav Egorov (Google), 2017-01-30 18:30:28]

lgtm

https://codereview.chromium.org/2659363003/diff/20001/runtime/vm/kernel_to_il.cc
File runtime/vm/kernel_to_il.cc (right):

https://codereview.chromium.org/2659363003/diff/20001/runtime/vm/kernel_to_il...
runtime/vm/kernel_to_il.cc:1785: MaybeCheckBool();
Consider making a helper:

  EvaluateBooleanExpression()

which does:

  EvaluateExpression(node->left());
  MaybeCheckBool();

https://codereview.chromium.org/2659363003/diff/20001/runtime/vm/kernel_to_il...
runtime/vm/kernel_to_il.cc:2546: instructions += MaybeCheckReturnType();
Maybe "maybe" is not a good prefix because it does not tell when this is
happening.

https://codereview.chromium.org/2659363003/diff/20001/runtime/vm/kernel_to_il...
runtime/vm/kernel_to_il.cc:5260: // FIXME(checked-mode)
Needs to be 

// TODO(issue-number) some comment

https://codereview.chromium.org/2659363003/diff/20001/runtime/vm/kernel_to_il.h
File runtime/vm/kernel_to_il.h (right):

https://codereview.chromium.org/2659363003/diff/20001/runtime/vm/kernel_to_il...
runtime/vm/kernel_to_il.h:477: void MaybeCheckBool() {
I would call it AssertBoolInCheckedMode() to match IL instruction name.

[kustermann, 2017-01-31 10:37:29]

Patchset #3 (id:40001) has been deleted

[kustermann, 2017-01-31 10:39:46]

https://codereview.chromium.org/2659363003/diff/20001/runtime/vm/kernel_to_il.cc
File runtime/vm/kernel_to_il.cc (right):

https://codereview.chromium.org/2659363003/diff/20001/runtime/vm/kernel_to_il...
runtime/vm/kernel_to_il.cc:1785: MaybeCheckBool();
On 2017/01/30 18:30:28, Vyacheslav Egorov (Google) wrote:
> Consider making a helper:
> 
>   EvaluateBooleanExpression()
> 
> which does:
> 
>   EvaluateExpression(node->left());
>   MaybeCheckBool();

Done.

https://codereview.chromium.org/2659363003/diff/20001/runtime/vm/kernel_to_il...
runtime/vm/kernel_to_il.cc:2546: instructions += MaybeCheckReturnType();
On 2017/01/30 18:30:28, Vyacheslav Egorov (Google) wrote:
> Maybe "maybe" is not a good prefix because it does not tell when this is
> happening.
> 

Done.

https://codereview.chromium.org/2659363003/diff/20001/runtime/vm/kernel_to_il...
runtime/vm/kernel_to_il.cc:5260: // FIXME(checked-mode)
On 2017/01/30 18:30:28, Vyacheslav Egorov (Google) wrote:
> Needs to be 
> 
> // TODO(issue-number) some comment

I'll remove the TODO. The ConstantEvaluator has been modified to result in a
compile time error if the types don't match.

https://codereview.chromium.org/2659363003/diff/20001/runtime/vm/kernel_to_il.h
File runtime/vm/kernel_to_il.h (right):

https://codereview.chromium.org/2659363003/diff/20001/runtime/vm/kernel_to_il...
runtime/vm/kernel_to_il.h:477: void MaybeCheckBool() {
On 2017/01/30 18:30:28, Vyacheslav Egorov (Google) wrote:
> I would call it AssertBoolInCheckedMode() to match IL instruction name.

Done.

[kustermann, 2017-01-31 11:08:12]

Patchset #4 (id:80001) has been deleted

[kustermann, 2017-01-31 11:10:53]

Description was changed from

==========
VM: [Kernel] Partial support for checked mode.
==========

to

==========
VM: [Kernel] Partial support for checked mode.

R=vegorov@google.com

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

[kustermann, 2017-01-31 11:10:57]

Committed patchset #3 (id:60001) manually as
fc2458b8423979573ef3bdc70edf9711b38a99b2 (presubmit successful).