Implement assert in initializer list in VM.

See: https://docs.google.com/document/d/1iEuLbDMgbB657R5aFj9EfnT6B0obzQk93Of7CfWKc00/edit#heading=h.9ixvz54wwawi

Implement assert in initializer list in VM.

R=hausner@google.com

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

[Lasse Reichstein Nielsen, 2016-05-23 09:10:03]

lrn@google.com changed reviewers:
+ eernst@google.com, floitsch@google.com, leafp@google.com, rnystrom@google.com

[Lasse Reichstein Nielsen, 2016-05-23 09:10:03]

Turns out to be extremely easy since the VM converts initializer lists to
statements already.

[eernst, 2016-05-23 09:18:30]

On 2016/05/23 09:10:03, Lasse Reichstein Nielsen wrote:
> Turns out to be extremely easy since the VM converts initializer lists to
> statements already.

And the checks that the asserted expressions are const in a const
constructor are already in place, or 'proof-of-concept' means that
we don't care for now?

[Lasse Reichstein Nielsen, 2016-05-23 09:47:24]

On 2016/05/23 09:18:30, eernst wrote:
> And the checks that the asserted expressions are const in a const
> constructor are already in place, or 'proof-of-concept' means that
> we don't care for now?

Good point. Is now checked.

[eernst, 2016-05-23 10:32:47]

On 2016/05/23 09:47:24, Lasse Reichstein Nielsen wrote:
> On 2016/05/23 09:18:30, eernst wrote:
> > And the checks that the asserted expressions are const in a const
> > constructor are already in place, or 'proof-of-concept' means that
> > we don't care for now?
> 
> Good point. Is now checked.

Thanks!

NB: I should not L G T M a CL on the vm because I have too little 
insight into its overall architecture to know whether any given CL
will fit well, so I'll leave that step to others.

[Lasse Reichstein Nielsen, 2016-07-07 12:43:31]

Description was changed from

==========
Proof-of-concept: Implement assert in initializer list in VM.
==========

to

==========
Implement assert in initializer list in VM.
==========

[Lasse Reichstein Nielsen, 2016-07-07 12:44:06]

Description was changed from

==========
Implement assert in initializer list in VM.
==========

to

==========
See:
https://docs.google.com/document/d/1iEuLbDMgbB657R5aFj9EfnT6B0obzQk93Of7CfWKc...

Implement assert in initializer list in VM.
==========

[Lasse Reichstein Nielsen, 2016-07-07 12:45:49]

lrn@google.com changed reviewers:
+ asiva@google.com, hausner@google.com

[Lasse Reichstein Nielsen, 2016-07-07 12:45:50]

@asiva, @hausner
Please take a look at this and the design document.
If there are any questions to the design, please chime in, and if you have a
better implementation, that would also be great.

[Lasse Reichstein Nielsen, 2016-07-07 12:48:41]

https://codereview.chromium.org/2002923002/diff/60001/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/2002923002/diff/60001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:9227: // but ensure that the value is a bool.
Another option is to have a separate function that only does the throwing.

A third option is to replace the condition with `!!condition` before calling 
the function instead of using an if, which enforces booleanness at the call
point and lets the function call work as usual on that bool value.

[siva, 2016-07-12 16:27:02]

What is the status of this CL, is it ready for landing in the VM or are you
still experimenting with options.

If the CL is ready for landing I can take a look at it as Mathias is on
vacation.

[Lasse Reichstein Nielsen, 2016-07-12 17:16:37]

I believe the CL is ready for review and landing.

It matches the current design document and the tests run.
I would recommend reading the design document as well, so it's clear what the
goal is.

[Lasse Reichstein Nielsen, 2016-08-15 10:01:35]

Ping

[hausner, 2016-08-15 20:54:27]

See comments.

https://codereview.chromium.org/2002923002/diff/60001/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/2002923002/diff/60001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:2596: if (FLAG_assert_initializer && CurrentToken() ==
Token::kASSERT) {
Extra parens around the == expression.

https://codereview.chromium.org/2002923002/diff/60001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:3601: } else if (FLAG_assert_initializer && CurrentToken()
== Token::kASSERT) {
Extra parens around the == expression.

https://codereview.chromium.org/2002923002/diff/60001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:9227: // but ensure that the value is a bool.
On 2016/07/07 12:48:41, Lasse Reichstein Nielsen wrote:
> Another option is to have a separate function that only does the throwing.
> 
> A third option is to replace the condition with `!!condition` before calling 
> the function instead of using an if, which enforces booleanness at the call
> point and lets the function call work as usual on that bool value.

Maybe option 2 is cleaner. I'd prefer a separate function for the const case.

https://codereview.chromium.org/2002923002/diff/60001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:9230: arguments->Add(new(Z) LiteralNode(condition_pos,
Bool::False()));
I don't see a bool parameter in _AssertionError._checkAssertion()


  static void _checkAssertion(condition, int start, int end) {
    if (condition is Function) {
      condition = condition();
    }
    if (!condition) {
      _throwNew(start, end);
    }
  }

https://codereview.chromium.org/2002923002/diff/60001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:9246: condition,
I don't think you can use the 'condition' expression twice (once as a parameter
to checkAssertion(), and once negated in the if-condition.) You need to use a
temporary variable to hold the evaluated expression.

[Lasse Reichstein Nielsen, 2016-08-23 07:05:31]

https://codereview.chromium.org/2002923002/diff/60001/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/2002923002/diff/60001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:3601: } else if (FLAG_assert_initializer && CurrentToken()
== Token::kASSERT) {
Done.

https://codereview.chromium.org/2002923002/diff/60001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:9227: // but ensure that the value is a bool.
Will do.

https://codereview.chromium.org/2002923002/diff/60001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:9230: arguments->Add(new(Z) LiteralNode(condition_pos,
Bool::False()));
Since I already checked the condition in the `IfNode`, I don't pass the
condition to the helper function, just a `false` value. It's still the
"condition" parameter. 
That also means that I don't use the condition expression twice.
(Moot now, since I changed it to just call a different function).

https://codereview.chromium.org/2002923002/diff/60001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:9246: condition,
I don't, as explained above.

[Lasse Reichstein Nielsen, 2016-08-23 07:05:39]

PTAL

[hausner, 2016-08-23 16:21:34]

LGTM now.

[Lasse Reichstein Nielsen, 2016-08-24 10:43:34]

Description was changed from

==========
See:
https://docs.google.com/document/d/1iEuLbDMgbB657R5aFj9EfnT6B0obzQk93Of7CfWKc...

Implement assert in initializer list in VM.
==========

to

==========
See:
https://docs.google.com/document/d/1iEuLbDMgbB657R5aFj9EfnT6B0obzQk93Of7CfWKc...

Implement assert in initializer list in VM.

R=hausner@google.com

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

[Lasse Reichstein Nielsen, 2016-08-24 10:43:37]

Committed patchset #5 (id:80001) manually as
99439a336a47e34532c3c3ed0b001a54ed1069c9 (presubmit successful).