Add optional message argument to assert statements in the VM.

Add optional message argument to assert statements in the VM.

Add flag --assert-message to control the feature.


BUG=#24215
R=regis@google.com

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

[hausner, 2016-12-13 22:52:07]

Description was changed from

==========
Add optional message argument to assert statements in the VM.

Add flag --assert-message to control the feature.


BUG=#24215
==========

to

==========
Add optional message argument to assert statements in the VM.

Add flag --assert-message to control the feature.


BUG=#24215
==========

[hausner, 2016-12-13 22:52:07]

hausner@google.com changed reviewers:
+ regis@google.com

[regis, 2016-12-13 23:26:52]

Still a few open questions.

https://codereview.chromium.org/2574003003/diff/1/runtime/lib/errors_patch.dart
File runtime/lib/errors_patch.dart (right):

https://codereview.chromium.org/2574003003/diff/1/runtime/lib/errors_patch.da...
runtime/lib/errors_patch.dart:28: static _doThrowNew(int assertionStart, int
assertionEnd, Object message)
Why do we need this new function _doThrowNew called from _throwNew?
If necessary, explain in a comment.

https://codereview.chromium.org/2574003003/diff/1/runtime/lib/errors_patch.da...
runtime/lib/errors_patch.dart:31: static _checkAssertion(condition) {
This is not checking the assertion anymore (no throw). Rename to
_evaluateCondition?

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

https://codereview.chromium.org/2574003003/diff/1/runtime/vm/parser.cc#newcod...
runtime/vm/parser.cc:9168: if (FLAG_assert_message && CurrentToken() ==
Token::kCOMMA) {
No parens?

https://codereview.chromium.org/2574003003/diff/1/runtime/vm/parser.cc#newcod...
runtime/vm/parser.cc:9247: // Build if (!condtition)
_AsertionError._throwNew(...)
condtition -> condition

https://codereview.chromium.org/2574003003/diff/1/tests/language/assert_messa...
File tests/language/assert_message_test.dart (right):

https://codereview.chromium.org/2574003003/diff/1/tests/language/assert_messa...
tests/language/assert_message_test.dart:23: // Successful asserts won't execute
message.
Is this still true when await is involved?

https://codereview.chromium.org/2574003003/diff/1/tests/language/assert_messa...
tests/language/assert_message_test.dart:77: assert(await true, await 2);
How about
assert(await true, throw "unreachable");
and
assert(await true, await throw "unreachable");

[hausner, 2016-12-14 16:59:36]

PTAL.

https://codereview.chromium.org/2574003003/diff/1/runtime/lib/errors_patch.dart
File runtime/lib/errors_patch.dart (right):

https://codereview.chromium.org/2574003003/diff/1/runtime/lib/errors_patch.da...
runtime/lib/errors_patch.dart:28: static _doThrowNew(int assertionStart, int
assertionEnd, Object message)
On 2016/12/13 23:26:51, regis wrote:
> Why do we need this new function _doThrowNew called from _throwNew?
> If necessary, explain in a comment.

Eliminated _doThrowNew

https://codereview.chromium.org/2574003003/diff/1/runtime/lib/errors_patch.da...
runtime/lib/errors_patch.dart:31: static _checkAssertion(condition) {
On 2016/12/13 23:26:51, regis wrote:
> This is not checking the assertion anymore (no throw). Rename to
> _evaluateCondition?

Done.

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

https://codereview.chromium.org/2574003003/diff/1/runtime/vm/parser.cc#newcod...
runtime/vm/parser.cc:9168: if (FLAG_assert_message && CurrentToken() ==
Token::kCOMMA) {
On 2016/12/13 23:26:51, regis wrote:
> No parens?

Added parens. The formatter was happy without them, though.

https://codereview.chromium.org/2574003003/diff/1/runtime/vm/parser.cc#newcod...
runtime/vm/parser.cc:9247: // Build if (!condtition)
_AsertionError._throwNew(...)
On 2016/12/13 23:26:51, regis wrote:
> condtition -> condition

Done.

https://codereview.chromium.org/2574003003/diff/1/tests/language/assert_messa...
File tests/language/assert_message_test.dart (right):

https://codereview.chromium.org/2574003003/diff/1/tests/language/assert_messa...
tests/language/assert_message_test.dart:23: // Successful asserts won't execute
message.
On 2016/12/13 23:26:51, regis wrote:
> Is this still true when await is involved?

Yes. Added test case below in the async test cases.

https://codereview.chromium.org/2574003003/diff/1/tests/language/assert_messa...
tests/language/assert_message_test.dart:77: assert(await true, await 2);
On 2016/12/13 23:26:51, regis wrote:
> How about
> assert(await true, throw "unreachable");
> and
> assert(await true, await throw "unreachable");

Done.

[regis, 2016-12-14 17:28:55]

lgtm

[hausner, 2016-12-14 18:30:24]

Thank you.

https://codereview.chromium.org/2574003003/diff/40001/runtime/lib/errors_patc...
File runtime/lib/errors_patch.dart (right):

https://codereview.chromium.org/2574003003/diff/40001/runtime/lib/errors_patc...
runtime/lib/errors_patch.dart:28: static _throwNew(int assertionStart, int
assertionEnd, Object message) {
Turns out the indirection through a Dart method is necessary. Added it back.

[hausner, 2016-12-14 19:05:23]

Description was changed from

==========
Add optional message argument to assert statements in the VM.

Add flag --assert-message to control the feature.


BUG=#24215
==========

to

==========
Add optional message argument to assert statements in the VM.

Add flag --assert-message to control the feature.


BUG=#24215
R=regis@google.com

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

[hausner, 2016-12-14 19:05:24]

Committed patchset #4 (id:60001) manually as
2c0b605a961bfe0ef6919e43489870df99791325 (presubmit successful).

[Lasse Reichstein Nielsen, 2016-12-14 20:47:17]

lrn@google.com changed reviewers:
+ lrn@google.com

[Lasse Reichstein Nielsen, 2016-12-14 20:47:18]

https://codereview.chromium.org/2574003003/diff/40001/runtime/lib/errors_patc...
File runtime/lib/errors_patch.dart (right):

https://codereview.chromium.org/2574003003/diff/40001/runtime/lib/errors_patc...
runtime/lib/errors_patch.dart:28: static _throwNew(int assertionStart, int
assertionEnd, Object message) {
Yes, I don't know why, but I couldn't get it working without the extra
indirection.