Move Null class out of the VM isolate.

Move Null class out of the VM isolate.

BUG=http://dartbug.com/12071
BUG=http://dartbug.com/12128
BUG=http://dartbug.com/12319
BUG=http://dartbug.com/12337
R=iposva@google.com

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

[rmacnak, 2013-08-09 17:45:43]

https://chromiumcodereview.appspot.com/22638011/diff/12001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://chromiumcodereview.appspot.com/22638011/diff/12001/runtime/vm/object....
runtime/vm/object.cc:422: // Allocate and initialize the sentinel values of Null
class.
There are actually five instances of Null >:/

https://chromiumcodereview.appspot.com/22638011/diff/12001/runtime/vm/object....
runtime/vm/object.cc:1106: // The classes 'Null' and 'void' are not registered
in the class dictionary,
Null is not a reserved word; it just isn't exported by dart:core.

https://chromiumcodereview.appspot.com/22638011/diff/12001/runtime/vm/object....
runtime/vm/object.cc:1108: // allocated, because the classes reside in the VM
isolate.
Not sure why this would prevent heap allocation. If the classes are in the VM
isolate, surely it would be safe to have their names allocated in the VM
isolate.  Moot at this point, but still odd.

https://chromiumcodereview.appspot.com/22638011/diff/12001/runtime/vm/object....
runtime/vm/object.cc:10457: return HasResolvedTypeClass() &&
This mimics Bool, but perhaps type_class().IsNullClass() ?

[Ivan Posva, 2013-08-09 21:15:12]

LGTM -ip

https://chromiumcodereview.appspot.com/22638011/diff/12001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://chromiumcodereview.appspot.com/22638011/diff/12001/runtime/vm/object....
runtime/vm/object.cc:1108: // allocated, because the classes reside in the VM
isolate.
On 2013/08/09 17:45:44, Ryan Macnak wrote:
> Not sure why this would prevent heap allocation. If the classes are in the VM
> isolate, surely it would be safe to have their names allocated in the VM
> isolate.  Moot at this point, but still odd.

It looks like this comment is out-of-date. The name is indeed allocated in the
VM isolate (see Symbols::Null).

[rmacnak, 2013-08-09 21:54:38]

Committed patchset #2 manually as r25982 (presubmit successful).

[siva, 2013-08-10 00:43:34]

https://chromiumcodereview.appspot.com/22638011/diff/18001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://chromiumcodereview.appspot.com/22638011/diff/18001/runtime/vm/object....
runtime/vm/object.cc:1115: // The class 'Null' is not register in the class
dictionary because it is not
registered, the comment also doesn't seem correct.

https://chromiumcodereview.appspot.com/22638011/diff/18001/runtime/vm/object....
runtime/vm/object.cc:1120: // is a built-in identifier (this is wrong).
Maybe you should add an issue to track this and add a TODO here.

https://chromiumcodereview.appspot.com/22638011/diff/18001/runtime/vm/object....
runtime/vm/object.cc:1126: // Consider removing when/if Null becomes an ordinary
class.
I think this should also have an issue and a TODO (probably in sync with comment
above about Null class being added to the pending_classes list.

https://chromiumcodereview.appspot.com/22638011/diff/18001/runtime/vm/object....
runtime/vm/object.cc:10466: (type_class() ==
Type::Handle(Type::NullType()).type_class());
This has become an expensive operation now with the need to create
a Handle inside of the function.

Wouldn't the NullType be always canonicalized and we could just compare the type
objects?

[rmacnak, 2013-08-12 19:53:26]

On 2013/08/10 00:43:34, siva wrote:
>
https://chromiumcodereview.appspot.com/22638011/diff/18001/runtime/vm/object.cc
> File runtime/vm/object.cc (right):
> 
>
https://chromiumcodereview.appspot.com/22638011/diff/18001/runtime/vm/object....
> runtime/vm/object.cc:1115: // The class 'Null' is not register in the class
> dictionary because it is not
> registered, the comment also doesn't seem correct.
> 
>
https://chromiumcodereview.appspot.com/22638011/diff/18001/runtime/vm/object....
> runtime/vm/object.cc:1120: // is a built-in identifier (this is wrong).
> Maybe you should add an issue to track this and add a TODO here.
> 
>
https://chromiumcodereview.appspot.com/22638011/diff/18001/runtime/vm/object....
> runtime/vm/object.cc:1126: // Consider removing when/if Null becomes an
ordinary
> class.
> I think this should also have an issue and a TODO (probably in sync with
comment
> above about Null class being added to the pending_classes list.
> 
>
https://chromiumcodereview.appspot.com/22638011/diff/18001/runtime/vm/object....
> runtime/vm/object.cc:10466: (type_class() ==
> Type::Handle(Type::NullType()).type_class());
> This has become an expensive operation now with the need to create
> a Handle inside of the function.
> 
> Wouldn't the NullType be always canonicalized and we could just compare the
type
> objects?

Addressed in https://codereview.chromium.org/22902002/.