Added builtin native call to exit the running vm

Added builtin native call to exit the running vm

Total uses exit(int) to signal whether to stop or restart

R=iposva@google.com
BUG=
TEST=

Committed: https://code.google.com/p/dart/source/detail?r=893

[rchandia, 2011-10-17 21:55:19]

[+vm-dev]

[rchandia, 2011-10-19 21:05:57]

On 2011/10/17 21:55:19, rchandia wrote:
> [+vm-dev]

Ping

[Ivan Posva, 2011-10-19 21:57:11]

http://codereview.chromium.org/8317006/diff/1/runtime/bin/builtin.cc
File runtime/bin/builtin.cc (right):

http://codereview.chromium.org/8317006/diff/1/runtime/bin/builtin.cc#newcode1
runtime/bin/builtin.cc:1: // Copyright (c) 2011, the Dart project authors. 
Please see the AUTHORS file
Please update to the latest revision of this file.

http://codereview.chromium.org/8317006/diff/1/runtime/bin/builtin.cc#newcode5
runtime/bin/builtin.cc:5: #include "bin/builtin.h"
System includes first, projects includes after.

http://codereview.chromium.org/8317006/diff/1/runtime/bin/builtin.dart
File runtime/bin/builtin.dart (right):

http://codereview.chromium.org/8317006/diff/1/runtime/bin/builtin.dart#newcode12
runtime/bin/builtin.dart:12: _Runtime._exit(status);
If called like this:
exit("0");
The VM will run into an assertion, instead you should check that only arguments
with the correct type are forwarded into native code:

void exit(int status) {
  if (status is !int) {
    throw IllegalArgumentException("int status expected");
  }
  _exit(status);
}

_exit(int status) native "_exit";

http://codereview.chromium.org/8317006/diff/1/runtime/bin/builtin.dart#newcode19
runtime/bin/builtin.dart:19: class _Runtime {
There is no need to add a class as a wrapper here. A top-level native function
_exit would be good enough. Eventually we should do the same thing for Logger,
which only exists due to legacy (all of the Logger functionality has been
gutted.)

[rchandia, 2011-10-21 17:29:41]

http://codereview.chromium.org/8317006/diff/1/runtime/bin/builtin.cc
File runtime/bin/builtin.cc (right):

http://codereview.chromium.org/8317006/diff/1/runtime/bin/builtin.cc#newcode5
runtime/bin/builtin.cc:5: #include "bin/builtin.h"
On 2011/10/19 21:57:11, Ivan Posva wrote:
> System includes first, projects includes after.

Done.

http://codereview.chromium.org/8317006/diff/1/runtime/bin/builtin.dart
File runtime/bin/builtin.dart (right):

http://codereview.chromium.org/8317006/diff/1/runtime/bin/builtin.dart#newcode12
runtime/bin/builtin.dart:12: _Runtime._exit(status);
On 2011/10/19 21:57:11, Ivan Posva wrote:
> If called like this:
> exit("0");
> The VM will run into an assertion, instead you should check that only
arguments
> with the correct type are forwarded into native code:
> 
> void exit(int status) {
>   if (status is !int) {
>     throw IllegalArgumentException("int status expected");
>   }
>   _exit(status);
> }
> 
> _exit(int status) native "_exit";

Done.

http://codereview.chromium.org/8317006/diff/1/runtime/bin/builtin.dart#newcode19
runtime/bin/builtin.dart:19: class _Runtime {
On 2011/10/19 21:57:11, Ivan Posva wrote:
> There is no need to add a class as a wrapper here. A top-level native function
> _exit would be good enough. Eventually we should do the same thing for Logger,
> which only exists due to legacy (all of the Logger functionality has been
> gutted.)

Done.

[siva, 2011-10-21 22:04:09]

http://codereview.chromium.org/8317006/diff/6001/runtime/bin/builtin.dart
File runtime/bin/builtin.dart (right):

http://codereview.chromium.org/8317006/diff/6001/runtime/bin/builtin.dart#new...
runtime/bin/builtin.dart:18: _exit(int status) native "Runtime_Exit";
The convention for native method names so far seems to be
<class_name>_<method_name> you are deviating from that style. Since this is a
top level method you could probably just use Exit

http://codereview.chromium.org/8317006/diff/6001/samples/total/src/TotalServe...
File samples/total/src/TotalServer.dart (right):

http://codereview.chromium.org/8317006/diff/6001/samples/total/src/TotalServe...
samples/total/src/TotalServer.dart:67: exit(0);
Will this test be run under dartium? if yes you need to give them a heads up
that print/exit needs to be implemented by them in order for this to work.

There is a change in works for injecting an implementation of print from the
embedder, I presume exit should be handled similarly (see 5476054 Dartium needs
to provide print in dart:core)

[Ivan Posva, 2011-10-21 23:05:21]

-Ivan

http://codereview.chromium.org/8317006/diff/6001/runtime/bin/builtin.cc
File runtime/bin/builtin.cc (right):

http://codereview.chromium.org/8317006/diff/6001/runtime/bin/builtin.cc#newco...
runtime/bin/builtin.cc:34: Dart_ExitScope();
Needs to move above exit to really make any sense.

[rchandia, 2011-10-25 14:37:50]

http://codereview.chromium.org/8317006/diff/6001/runtime/bin/builtin.cc
File runtime/bin/builtin.cc (right):

http://codereview.chromium.org/8317006/diff/6001/runtime/bin/builtin.cc#newco...
runtime/bin/builtin.cc:34: Dart_ExitScope();
On 2011/10/21 23:05:22, Ivan Posva wrote:
> Needs to move above exit to really make any sense.

Done.

http://codereview.chromium.org/8317006/diff/6001/runtime/bin/builtin.dart
File runtime/bin/builtin.dart (right):

http://codereview.chromium.org/8317006/diff/6001/runtime/bin/builtin.dart#new...
runtime/bin/builtin.dart:18: _exit(int status) native "Runtime_Exit";
On 2011/10/21 22:04:09, asiva wrote:
> The convention for native method names so far seems to be
> <class_name>_<method_name> you are deviating from that style. Since this is a
> top level method you could probably just use Exit

Done.

http://codereview.chromium.org/8317006/diff/6001/samples/total/src/TotalServe...
File samples/total/src/TotalServer.dart (right):

http://codereview.chromium.org/8317006/diff/6001/samples/total/src/TotalServe...
samples/total/src/TotalServer.dart:67: exit(0);
On 2011/10/21 22:04:09, asiva wrote:
> Will this test be run under dartium?

No, this is only server side. Client code should never need to exit with a
status code.

> There is a change in works for injecting an implementation of print from the
> embedder, I presume exit should be handled similarly (see 5476054 Dartium
needs
> to provide print in dart:core)

This being server side is that still the case?

[siva, 2011-10-25 17:19:34]

http://codereview.chromium.org/8317006/diff/6001/samples/total/src/TotalServe...
File samples/total/src/TotalServer.dart (right):

http://codereview.chromium.org/8317006/diff/6001/samples/total/src/TotalServe...
samples/total/src/TotalServer.dart:67: exit(0);
No, if this is server side only it is not an issue.

On 2011/10/25 14:37:50, rchandia wrote:
> On 2011/10/21 22:04:09, asiva wrote:
> > Will this test be run under dartium?
> 
> No, this is only server side. Client code should never need to exit with a
> status code.
> 
> > There is a change in works for injecting an implementation of print from the
> > embedder, I presume exit should be handled similarly (see 5476054 Dartium
> needs
> > to provide print in dart:core)
> 
> This being server side is that still the case?

[rchandia, 2011-10-27 13:48:43]

Ping

[Ivan Posva, 2011-10-27 14:07:55]

LGTM with comment.

-Ivan

http://codereview.chromium.org/8317006/diff/12001/runtime/bin/builtin.dart
File runtime/bin/builtin.dart (right):

http://codereview.chromium.org/8317006/diff/12001/runtime/bin/builtin.dart#ne...
runtime/bin/builtin.dart:12: if (!(status is int)) {
This reads a bit easier:
if (status is !int) {

[rchandia, 2011-10-28 15:33:43]

http://codereview.chromium.org/8317006/diff/12001/runtime/bin/builtin.dart
File runtime/bin/builtin.dart (right):

http://codereview.chromium.org/8317006/diff/12001/runtime/bin/builtin.dart#ne...
runtime/bin/builtin.dart:12: if (!(status is int)) {
On 2011/10/27 14:07:55, Ivan Posva wrote:
> This reads a bit easier:
> if (status is !int) {

Done.

[rchandia, 2011-10-28 15:34:37]

submitted as r893