Change List constructors.

Change List constructors.

BUG=http://dartbug.com/1331
BUG=http://dartbug.com/788

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

[floitsch, 2012-11-07 14:15:13]

@Nicolas: could you please verify that the change to the optimizer is correct?

[ngeoffray, 2012-11-07 14:27:52]

https://codereview.chromium.org/11312122/diff/16/sdk/lib/_internal/compiler/i...
File sdk/lib/_internal/compiler/implementation/ssa/optimize.dart (right):

https://codereview.chromium.org/11312122/diff/16/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/ssa/optimize.dart:256: DartType
defaultClass = compiler.listClass.defaultClass;
Please add a TODO that this should be cached.

https://codereview.chromium.org/11312122/diff/16/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/ssa/optimize.dart:257: if
(defaultClass == null) return false;
Also check if element is a constructor.

https://codereview.chromium.org/11312122/diff/16/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/ssa/optimize.dart:264: &&
element.enclosingElement.declaration == defaultClass.element;
Do this check earlier.

[floitsch, 2012-11-07 16:42:48]

PTAL. thanks.

https://codereview.chromium.org/11312122/diff/16/sdk/lib/_internal/compiler/i...
File sdk/lib/_internal/compiler/implementation/ssa/optimize.dart (right):

https://codereview.chromium.org/11312122/diff/16/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/ssa/optimize.dart:256: DartType
defaultClass = compiler.listClass.defaultClass;
On 2012/11/07 14:27:52, ngeoffray wrote:
> Please add a TODO that this should be cached.

Done.

https://codereview.chromium.org/11312122/diff/16/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/ssa/optimize.dart:257: if
(defaultClass == null) return false;
On 2012/11/07 14:27:52, ngeoffray wrote:
> Also check if element is a constructor.

Done.

https://codereview.chromium.org/11312122/diff/16/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/ssa/optimize.dart:264: &&
element.enclosingElement.declaration == defaultClass.element;
On 2012/11/07 14:27:52, ngeoffray wrote:
> Do this check earlier.

Done.

[sra1, 2012-11-07 21:36:34]

DBC

https://codereview.chromium.org/11312122/diff/2015/sdk/lib/core/list.dart
File sdk/lib/core/list.dart (right):

https://codereview.chromium.org/11312122/diff/2015/sdk/lib/core/list.dart#new...
sdk/lib/core/list.dart:12: * Creates an extendable list of the given [length].
'extendable' is not quite right since it can be shrunk.

https://codereview.chromium.org/11312122/diff/2015/sdk/lib/core/list.dart#new...
sdk/lib/core/list.dart:26: List.filled(int length, E fill);
It is a pity that 'fill' is a named parameter in one constructor and part of the
name in another.

As I programmer I would like to be able to write:

new List();
new List(10);
new List(10, fill: 0);

new List.fixedLength(10);
new List.fixedLength(10, fill: 0);

I'd be happy to remove new List() and recommend writing the more concise [] or
<T>[].

https://codereview.chromium.org/11312122/diff/2015/sdk/lib/core/list.dart#new...
sdk/lib/core/list.dart:29: * Creates an extandable list with the elements of
[other]. The order in
extEndable, if you keep the name.

https://codereview.chromium.org/11312122/diff/2015/sdk/lib/core/list.dart#new...
sdk/lib/core/list.dart:181: void insertRange(int start, int length, [E
initialValue]);
Should initialValue be renamed to fill?

[Lasse Reichstein Nielsen, 2012-11-12 11:31:05]

I think you need to change some code in flow_graph_builder.cc:
GetResultCidOfConstructor too.
It's probably safe now, but you can make it more precise by saying that the List
constructor always returns a kGrowableObjectArrayCid.

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

https://codereview.chromium.org/11312122/diff/2015/runtime/lib/array_patch.da...
runtime/lib/array_patch.dart:14: if (length != 0) {
Don't check, just set the length. It's correct for zero too.

https://codereview.chromium.org/11312122/diff/2015/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/lib/core_patch.dart (right):

https://codereview.chromium.org/11312122/diff/2015/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/core_patch.dart:160: throw new
ArgumentError("Length must be a positive integer: $length.");
Use Error.safeToString on 'length' in case it's not an int.

https://codereview.chromium.org/11312122/diff/2015/tests/language/list_test.dart
File tests/language/list_test.dart (right):

https://codereview.chromium.org/11312122/diff/2015/tests/language/list_test.d...
tests/language/list_test.dart:98: Expect.throws(() => new List.fixedLength(-1),
(e) => true);
Can't you just omit the exception-test method in this case?

[Lasse Reichstein Nielsen, 2012-11-12 11:31:15]

And LGTM.

[floitsch, 2012-11-12 13:21:50]

Adding Carl for the VM changes.

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

https://codereview.chromium.org/11312122/diff/2015/runtime/lib/array_patch.da...
runtime/lib/array_patch.dart:14: if (length != 0) {
On 2012/11/12 11:31:06, Lasse Reichstein Nielsen wrote:
> Don't check, just set the length. It's correct for zero too.

Done.

https://codereview.chromium.org/11312122/diff/2015/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/lib/core_patch.dart (right):

https://codereview.chromium.org/11312122/diff/2015/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/core_patch.dart:160: throw new
ArgumentError("Length must be a positive integer: $length.");
On 2012/11/12 11:31:06, Lasse Reichstein Nielsen wrote:
> Use Error.safeToString on 'length' in case it's not an int.

Done.

https://codereview.chromium.org/11312122/diff/2015/sdk/lib/core/list.dart
File sdk/lib/core/list.dart (right):

https://codereview.chromium.org/11312122/diff/2015/sdk/lib/core/list.dart#new...
sdk/lib/core/list.dart:12: * Creates an extendable list of the given [length].
On 2012/11/07 21:36:34, sra1 wrote:
> 'extendable' is not quite right since it can be shrunk.
> 
reworded.

https://codereview.chromium.org/11312122/diff/2015/sdk/lib/core/list.dart#new...
sdk/lib/core/list.dart:26: List.filled(int length, E fill);
On 2012/11/07 21:36:34, sra1 wrote:
> It is a pity that 'fill' is a named parameter in one constructor and part of
the
> name in another.
> 
> As I programmer I would like to be able to write:
> 
> new List();
> new List(10);
> new List(10, fill: 0);
> 
> new List.fixedLength(10);
> new List.fixedLength(10, fill: 0);
> 
> I'd be happy to remove new List() and recommend writing the more concise [] or
> <T>[].

Completely agree. There is bug open, but currently we can't do otherwise.

https://codereview.chromium.org/11312122/diff/2015/sdk/lib/core/list.dart#new...
sdk/lib/core/list.dart:29: * Creates an extandable list with the elements of
[other]. The order in
On 2012/11/07 21:36:34, sra1 wrote:
> extEndable, if you keep the name.

Reworded.

https://codereview.chromium.org/11312122/diff/2015/sdk/lib/core/list.dart#new...
sdk/lib/core/list.dart:181: void insertRange(int start, int length, [E
initialValue]);
On 2012/11/07 21:36:34, sra1 wrote:
> Should initialValue be renamed to fill?

Done.

https://codereview.chromium.org/11312122/diff/2015/tests/language/list_test.dart
File tests/language/list_test.dart (right):

https://codereview.chromium.org/11312122/diff/2015/tests/language/list_test.d...
tests/language/list_test.dart:98: Expect.throws(() => new List.fixedLength(-1),
(e) => true);
On 2012/11/12 11:31:06, Lasse Reichstein Nielsen wrote:
> Can't you just omit the exception-test method in this case?

Done.

[cshapiro, 2012-11-12 19:31:35]

It looks like the only changed VM file is flow_graph_builder.cc.  

I am guessing that the VM changes you want me to look at are the assertion
failure.  It is possible some other files might have (accidentally) been
omitted.  Please confirm either way.

Thanks!

[floitsch, 2012-11-12 20:04:43]

On 2012/11/12 19:31:35, cshapiro wrote:
> It looks like the only changed VM file is flow_graph_builder.cc.
Correct.  
> 
> I am guessing that the VM changes you want me to look at are the assertion
> failure.  It is possible some other files might have (accidentally) been
> omitted.  Please confirm either way.
Yes. I have no idea where the assertion failure comes from. I'm pretty sure no
other file has been omitted.
thanks.
> 
> Thanks!

[floitsch, 2012-12-10 20:21:36]

Finally landing.
Just had to run a script and replace all new List(..) with new
List.fixedLength(..) to get rid of the assertion failure (thanks Anders for
debugging that).