Move ListImplementation from coreimpl to core, as a private member.

Move ListImplementation from coreimpl to core, as a private member.


BUG=

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

[Anders Johnsen, 2012-10-23 13:25:38]

Srdjan,

I'm trying make the move of ListImplementation, but I get the following error
when running in checked mode:

runtime/vm/object.cc:2829: error: expected: (index >= 0) && (index < Length())
Aborted (core dumped)

on the following code:

main() {
  "lol".charCodes().map((c) {
      return c;
  });
}

Do you know what I might be missing?

Thank you!

[srdjan, 2012-10-23 16:21:28]

This program fails on ia32 in checked mode:

#3  0x00062225 in dart::DynamicAssertionHelper::Fail (this=0xbfffed78,
format=0x2a7ce0 "expected: %s") at assert.cc:40
#4  0x001b505e in dart::TypeArguments::TypeAddr (this=0xbffff528, index=1) at
object.cc:2829
#5  0x001aab56 in dart::TypeArguments::TypeAt (this=0xbffff528, index=1) at
object.cc:2729
#6  0x001af131 in dart::TypeParameter::InstantiateFrom (this=0xbffff648,
instantiator_type_arguments=@0xbffff528) at object.cc:8970
#7  0x001aa6cd in dart::TypeArguments::InstantiateFrom (this=0xbffff600,
instantiator_type_arguments=@0xbffff528) at object.cc:2799
#8  0x001c6e04 in dart::Instance::IsInstanceOf (this=0xbffff510,
other=@0xbffff518, other_instantiator=@0xbffff528, malformed_error=0xbffff558)
at object.cc:8127
#9  0x000d73f9 in DRT_HelperTypeCheck (isolate=0x87e400, arguments={isolate_ =
0x87e400, argc_ = 6, argv_ = 0xbffff798, retval_ = 0xbffff79c}) at
code_generator.cc:624
#10 0x000cc910 in dart::DRT_TypeCheck (arguments={isolate_ = 0x87e400, argc_ =
6, argv_ = 0xbffff798, retval_ = 0xbffff79c}) at code_generator.cc:608

CC-ing Regis who knows more about that.

- Srdjan

On 2012/10/23 13:25:38, ajohnsen wrote:
> Srdjan,
> 
> I'm trying make the move of ListImplementation, but I get the following error
> when running in checked mode:
> 
> runtime/vm/object.cc:2829: error: expected: (index >= 0) && (index < Length())
> Aborted (core dumped)
> 
> on the following code:
> 
> main() {
>   "lol".charCodes().map((c) {
>       return c;
>   });
> }
> 
> Do you know what I might be missing?
> 
> Thank you!

[Anders Johnsen, 2012-10-30 10:19:14]

This is now ready for review - the VM is fixed! :)

Mads, can you look at the VM changes?
Peter, can you look at the limited Dart2JS changes?

Thank you all!

- Anders

[Mads Ager (google), 2012-10-30 10:37:47]

https://codereview.chromium.org/11189141/diff/6015/lib/compiler/implementatio...
File lib/compiler/implementation/lib/core_patch.dart (right):

https://codereview.chromium.org/11189141/diff/6015/lib/compiler/implementatio...
lib/compiler/implementation/lib/core_patch.dart:154: patch class _ListImpl<E> {
Is there any reason to have _ListImpl at this point? Can we get rid of that in a
followup patch and just put external on the two list constructors in core and
patch it directly?

https://codereview.chromium.org/11189141/diff/6015/runtime/vm/intrinsifier_ia...
File runtime/vm/intrinsifier_ia32.cc (right):

https://codereview.chromium.org/11189141/diff/6015/runtime/vm/intrinsifier_ia...
runtime/vm/intrinsifier_ia32.cc:178: const Library& core_lib =
Library::Handle(Library::CoreLibrary());
Are these changes only needed on ia32? Aren't these intrinsified on x64?

[ahe, 2012-10-30 11:20:21]

https://codereview.chromium.org/11189141/diff/10002/lib/compiler/implementati...
File lib/compiler/implementation/lib/core_patch.dart (right):

https://codereview.chromium.org/11189141/diff/10002/lib/compiler/implementati...
lib/compiler/implementation/lib/core_patch.dart:1: // Copyright (c) 2012, the
Dart project authors.  Please see the AUTHORS file
This file LGTM.

https://codereview.chromium.org/11189141/diff/10002/lib/compiler/implementati...
File lib/compiler/implementation/lib/coreimpl_patch.dart (right):

https://codereview.chromium.org/11189141/diff/10002/lib/compiler/implementati...
lib/compiler/implementation/lib/coreimpl_patch.dart:1: // Copyright (c) 2012,
the Dart project authors.  Please see the AUTHORS file
This file LGTM.

[Anders Johnsen, 2012-10-30 11:53:37]

PTAL

https://codereview.chromium.org/11189141/diff/6015/lib/compiler/implementatio...
File lib/compiler/implementation/lib/core_patch.dart (right):

https://codereview.chromium.org/11189141/diff/6015/lib/compiler/implementatio...
lib/compiler/implementation/lib/core_patch.dart:154: patch class _ListImpl<E> {
On 2012/10/30 10:37:47, Mads Ager wrote:
> Is there any reason to have _ListImpl at this point? Can we get rid of that in
a
> followup patch and just put external on the two list constructors in core and
> patch it directly?

The next move is to make List an 'abstract class', so we can do the cleanup you
describe. But I want to do that in the second step, as it requires further fixes
in various places.

https://codereview.chromium.org/11189141/diff/6015/runtime/vm/intrinsifier_ia...
File runtime/vm/intrinsifier_ia32.cc (right):

https://codereview.chromium.org/11189141/diff/6015/runtime/vm/intrinsifier_ia...
runtime/vm/intrinsifier_ia32.cc:178: const Library& core_lib =
Library::Handle(Library::CoreLibrary());
On 2012/10/30 10:37:47, Mads Ager wrote:
> Are these changes only needed on ia32? Aren't these intrinsified on x64?

Well spotted, found a case in x64! Passes all tests on both platforms now.

[Mads Ager (google), 2012-10-30 11:58:43]

LGTM

https://codereview.chromium.org/11189141/diff/10002/runtime/lib/string_base.dart
File runtime/lib/string_base.dart (right):

https://codereview.chromium.org/11189141/diff/10002/runtime/lib/string_base.d...
runtime/lib/string_base.dart:23: // TODO(ajohnsen): Add fast path once
string_base is in core.
Do you have any idea how much the disabling of this fast case will cost?

[Anders Johnsen, 2012-10-30 12:02:13]

https://codereview.chromium.org/11189141/diff/10002/runtime/lib/string_base.dart
File runtime/lib/string_base.dart (right):

https://codereview.chromium.org/11189141/diff/10002/runtime/lib/string_base.d...
runtime/lib/string_base.dart:23: // TODO(ajohnsen): Add fast path once
string_base is in core.
On 2012/10/30 11:58:43, Mads Ager wrote:
> Do you have any idea how much the disabling of this fast case will cost?

TBH, no. But getting this fixed ASAP is a number one priority for me.

[floitsch, 2012-10-30 13:17:19]

LGTM.

https://codereview.chromium.org/11189141/diff/13001/runtime/lib/array_patch.dart
File runtime/lib/array_patch.dart (right):

https://codereview.chromium.org/11189141/diff/13001/runtime/lib/array_patch.d...
runtime/lib/array_patch.dart:18: var list = new _GrowableObjectArray<E>();
Keep type.

[Anders Johnsen, 2012-10-30 13:20:44]

Thanks, Landing.

https://codereview.chromium.org/11189141/diff/13001/runtime/lib/array_patch.dart
File runtime/lib/array_patch.dart (right):

https://codereview.chromium.org/11189141/diff/13001/runtime/lib/array_patch.d...
runtime/lib/array_patch.dart:18: var list = new _GrowableObjectArray<E>();
On 2012/10/30 13:17:19, floitsch wrote:
> Keep type.

Done.