Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(82)

Issue 1307363005: Add optional message argument to assert statements in the VM. (Closed)

Created:
5 years, 3 months ago by Lasse Reichstein Nielsen
Modified:
4 years ago
Reviewers:
siva, hausner, Ivan Posva
CC:
reviews_dartlang.org, vm-dev_dartlang.org, Paul Berry
Base URL:
https://github.com/dart-lang/sdk.git@master
Target Ref:
refs/heads/master
Visibility:
Public.

Description

Add optional message argument to assert statements in the VM. Add flag --assert-message to control the feature. Fixes issue #24215 BUG= http://dartbug.com/24215

Patch Set 1 #

Total comments: 6

Patch Set 2 : Merge to head. #

Patch Set 3 : Reimplement patch. #

Patch Set 4 : Found the bad merge. #

Patch Set 5 : Found and adapted a call of AssertionError._create in C+ code. #

Patch Set 6 : Add test. #

Patch Set 7 : Use correct flag in test. #

Patch Set 8 : More tests. #

Total comments: 3
Unified diffs Side-by-side diffs Delta from patch set Stats (+185 lines, -40 lines) Patch
M runtime/lib/errors.cc View 1 2 3 chunks +5 lines, -2 lines 0 comments Download
M runtime/lib/errors_patch.dart View 1 2 3 4 5 6 7 3 chunks +20 lines, -17 lines 0 comments Download
M runtime/vm/bootstrap_natives.h View 1 2 1 chunk +1 line, -1 line 0 comments Download
M runtime/vm/code_generator.cc View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download
M runtime/vm/parser.h View 1 2 1 chunk +3 lines, -0 lines 0 comments Download
M runtime/vm/parser.cc View 1 2 3 4 chunks +51 lines, -15 lines 0 comments Download
M runtime/vm/symbols.h View 1 2 3 1 chunk +0 lines, -1 line 0 comments Download
M sdk/lib/core/errors.dart View 1 2 1 chunk +3 lines, -1 line 1 comment Download
M sdk/lib/core/set.dart View 1 2 3 4 1 chunk +2 lines, -2 lines 0 comments Download
A tests/language/assert_message_test.dart View 1 2 3 4 5 6 7 1 chunk +99 lines, -0 lines 2 comments Download

Messages

Total messages: 31 (8 generated)
Lasse Reichstein Nielsen
https://codereview.chromium.org/1307363005/diff/1/runtime/lib/errors_patch.dart File runtime/lib/errors_patch.dart (right): https://codereview.chromium.org/1307363005/diff/1/runtime/lib/errors_patch.dart#newcode47 runtime/lib/errors_patch.dart:47: "'$_failedAssertion' $_messageString"; The format is negotiable. https://codereview.chromium.org/1307363005/diff/1/runtime/vm/parser.cc File runtime/vm/parser.cc ...
5 years, 3 months ago (2015-08-28 18:51:59 UTC) #2
Ivan Posva
Thanks for verifying that the proposed DEP is implementable. I would like to hold off ...
5 years, 3 months ago (2015-08-28 22:37:48 UTC) #3
Bob Nystrom
Obvious drive-by comment... tests?
5 years, 3 months ago (2015-09-01 15:22:30 UTC) #5
sra1
On 2015/08/28 22:37:48, Ivan Posva wrote: > Thanks for verifying that the proposed DEP is ...
5 years, 3 months ago (2015-09-02 17:55:51 UTC) #6
sra1
https://chromiumcodereview.appspot.com/1307363005/diff/1/sdk/lib/core/errors.dart File sdk/lib/core/errors.dart (right): https://chromiumcodereview.appspot.com/1307363005/diff/1/sdk/lib/core/errors.dart#newcode100 sdk/lib/core/errors.dart:100: final Object message; 1. Is the intention to also ...
5 years, 3 months ago (2015-09-02 18:00:27 UTC) #8
Lasse Reichstein Nielsen
https://chromiumcodereview.appspot.com/1307363005/diff/1/sdk/lib/core/errors.dart File sdk/lib/core/errors.dart (right): https://chromiumcodereview.appspot.com/1307363005/diff/1/sdk/lib/core/errors.dart#newcode100 sdk/lib/core/errors.dart:100: final Object message; On 2015/09/02 18:00:27, sra1 wrote: > ...
5 years, 3 months ago (2015-09-03 07:42:04 UTC) #9
sethladd
FWIW here's dart2js' CL https://codereview.chromium.org/1325843003/
5 years, 3 months ago (2015-09-03 14:58:38 UTC) #10
Ivan Posva
LGTM and ready to submit once dart2js and analyzer are ready to commit. Thanks, -Ivan ...
5 years, 3 months ago (2015-09-04 06:39:31 UTC) #11
Paul Berry
FWIW here's the analyzer's CL: https://codereview.chromium.org/1309543011/ However we probably shouldn't land any of these changes ...
5 years, 3 months ago (2015-09-07 22:46:29 UTC) #13
Paul Berry
5 years, 3 months ago (2015-09-07 22:48:26 UTC) #15
sra1
The dart2js implementation is committed: https://codereview.chromium.org/1342213003/ You might like to modify the dart2js-specific tests to ...
5 years, 3 months ago (2015-09-17 03:52:33 UTC) #16
Lasse Reichstein Nielsen
The newest proposal requires errors in the message expression to be caught. The rewrite is ...
5 years, 1 month ago (2015-11-05 16:52:21 UTC) #17
Ivan Posva
On 2015/11/05 16:52:21, Lasse Reichstein Nielsen wrote: > The newest proposal requires errors in the ...
5 years, 1 month ago (2015-11-05 19:17:10 UTC) #18
Lasse Reichstein Nielsen
Reopen issue with updated implementation. The implementation must have a problem, it crashes on tests/compiler/dart2js_extras/assert_writh_message_test.dart ...
4 years, 1 month ago (2016-11-22 12:59:01 UTC) #19
Lasse Reichstein Nielsen
@asiva for an actual VM developer.
4 years, 1 month ago (2016-11-23 08:34:24 UTC) #22
Lasse Reichstein Nielsen
@hausner because it's parser related.
4 years ago (2016-11-23 11:27:45 UTC) #24
Lasse Reichstein Nielsen
https://codereview.chromium.org/1307363005/diff/140001/tests/language/assert_message_test.dart File tests/language/assert_message_test.dart (right): https://codereview.chromium.org/1307363005/diff/140001/tests/language/assert_message_test.dart#newcode77 tests/language/assert_message_test.dart:77: assert(await true, await 2); This test fails now. It ...
4 years ago (2016-11-24 09:37:41 UTC) #25
Lasse Reichstein Nielsen
https://codereview.chromium.org/1307363005/diff/140001/sdk/lib/core/errors.dart File sdk/lib/core/errors.dart (right): https://codereview.chromium.org/1307363005/diff/140001/sdk/lib/core/errors.dart#newcode99 sdk/lib/core/errors.dart:99: /** Message describing the assertion error. */ Still not ...
4 years ago (2016-11-24 15:13:37 UTC) #26
siva
Lasse is this CL ready for a review or are you asking us to take ...
4 years ago (2016-11-28 02:14:43 UTC) #27
Lasse Reichstein Nielsen
I *thought* it was ready, until I added a test that crashed :) If you ...
4 years ago (2016-11-28 06:29:15 UTC) #28
hausner
I created https://codereview.chromium.org/2564623003/ to check in this work.
4 years ago (2016-12-09 00:59:08 UTC) #29
siva
Can this CL be closed.
4 years ago (2016-12-21 00:24:05 UTC) #30
Lasse Reichstein Nielsen
4 years ago (2016-12-21 13:11:59 UTC) #31
On 2016/12/21 00:24:05, siva wrote:
> Can this CL be closed.

Yes, you created a new CL, so this one is stale. I'll close it.

Powered by Google App Engine
This is Rietveld 408576698