Start supporting fixed length and immutable lists.

Start supporting fixed length and immutable lists.

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

[floitsch, 2012-02-24 10:15:08]

LGTM.

https://chromiumcodereview.appspot.com/9453021/diff/1001/dart/frog/leg/lib/js...
File dart/frog/leg/lib/js_helper.dart (right):

https://chromiumcodereview.appspot.com/9453021/diff/1001/dart/frog/leg/lib/js...
dart/frog/leg/lib/js_helper.dart:582: checkFixedLength(receiver, 'addAll');
No need. receiver add does this check for you.
But I'm ok with keeping it.

https://chromiumcodereview.appspot.com/9453021/diff/1001/dart/frog/leg/ssa/co...
File dart/frog/leg/ssa/codegen.dart (right):

https://chromiumcodereview.appspot.com/9453021/diff/1001/dart/frog/leg/ssa/co...
dart/frog/leg/ssa/codegen.dart:801: if (node.isConst) {
You can keep this code (as otherwise you can't test this patch), but I will
remove it with my CTC changes.
const Arrays must go through the CTC as otherwise canonicalization is wrong.

[Lasse Reichstein Nielsen, 2012-02-24 11:08:41]

LGTM

https://chromiumcodereview.appspot.com/9453021/diff/1001/dart/frog/leg/lib/js...
File dart/frog/leg/lib/js_helper.dart (right):

https://chromiumcodereview.appspot.com/9453021/diff/1001/dart/frog/leg/lib/js...
dart/frog/leg/lib/js_helper.dart:245: checkImmutable(a, 'indexed set');
Seems it should be named "checkMutable" to match our other "check" methods.

https://chromiumcodereview.appspot.com/9453021/diff/1001/dart/frog/leg/lib/js...
dart/frog/leg/lib/js_helper.dart:311: checkFixedLength(list, reason) {
Also the opposite name here - whatever that would be, perhaps
"checkVariableLengthList".

I would prefer having different types for variable length lists and fixed length
lists (Vector?) at the library level, and if any subtype relation exists between
them, the variable length list should extend the fixed length list (which
should't be called anything with "fixed length", but should just not have any
methods that changes the length).

https://chromiumcodereview.appspot.com/9453021/diff/1001/dart/frog/leg/lib/js...
dart/frog/leg/lib/js_helper.dart:402: JS('bool', @'$0.fixed$length = $1',
result, true);
How about using "void" as type for things where we know that we don't use the
result?
Also, why pass true as parameter instead of using the JS "true" literal?

[ngeoffray, 2012-02-24 13:24:33]

LGTM

http://codereview.chromium.org/9453021/diff/1001/dart/frog/leg/ssa/nodes.dart
File dart/frog/leg/ssa/nodes.dart (right):

http://codereview.chromium.org/9453021/diff/1001/dart/frog/leg/ssa/nodes.dart...
dart/frog/leg/ssa/nodes.dart:2123: HLiteralList(inputs, this.isConst) :
super(inputs);
Please add a TODO(floitsch): remove 'isConst' once the CTC handles it.

http://codereview.chromium.org/9453021/diff/1001/dart/tests/corelib/corelib-l...
File dart/tests/corelib/corelib-leg.status (right):

http://codereview.chromium.org/9453021/diff/1001/dart/tests/corelib/corelib-l...
dart/tests/corelib/corelib-leg.status:43: RegExpAllMatchesTest: Fail, OK #
NoSuchMethodException : method not found: 'forEach'
Why is that OK now?

[ahe, 2012-02-27 11:51:11]

Hi Lasse, Nicolas, and Florian,

Thank you for your comments and suggestions.

I haven't merged with Florian's CTC changes. Once I do, I should be able to get
rid of the two TODOs I added in response to your comments.

Cheers,
Peter

https://chromiumcodereview.appspot.com/9453021/diff/1001/dart/frog/leg/lib/js...
File dart/frog/leg/lib/js_helper.dart (right):

https://chromiumcodereview.appspot.com/9453021/diff/1001/dart/frog/leg/lib/js...
dart/frog/leg/lib/js_helper.dart:245: checkImmutable(a, 'indexed set');
On 2012/02/24 11:08:41, Lasse Reichstein Nielsen wrote:
> Seems it should be named "checkMutable" to match our other "check" methods.

Done.

https://chromiumcodereview.appspot.com/9453021/diff/1001/dart/frog/leg/lib/js...
dart/frog/leg/lib/js_helper.dart:311: checkFixedLength(list, reason) {
On 2012/02/24 11:08:41, Lasse Reichstein Nielsen wrote:
> Also the opposite name here - whatever that would be, perhaps
> "checkVariableLengthList".

Done.

> I would prefer having different types for variable length lists and fixed
length
> lists (Vector?) at the library level, and if any subtype relation exists
between
> them, the variable length list should extend the fixed length list (which
> should't be called anything with "fixed length", but should just not have any
> methods that changes the length).

As we discussed in person: please file an RFE at dartbug.com.

https://chromiumcodereview.appspot.com/9453021/diff/1001/dart/frog/leg/lib/js...
dart/frog/leg/lib/js_helper.dart:402: JS('bool', @'$0.fixed$length = $1',
result, true);
On 2012/02/24 11:08:41, Lasse Reichstein Nielsen wrote:
> How about using "void" as type for things where we know that we don't use the
> result?

I'll see if that works.

> Also, why pass true as parameter instead of using the JS "true" literal?

I don't want to make assumptions about how we represent true.

https://chromiumcodereview.appspot.com/9453021/diff/1001/dart/frog/leg/lib/js...
dart/frog/leg/lib/js_helper.dart:582: checkFixedLength(receiver, 'addAll');
On 2012/02/24 10:15:09, floitsch wrote:
> No need. receiver add does this check for you.
> But I'm ok with keeping it.

Done.

https://chromiumcodereview.appspot.com/9453021/diff/1001/dart/frog/leg/ssa/co...
File dart/frog/leg/ssa/codegen.dart (right):

https://chromiumcodereview.appspot.com/9453021/diff/1001/dart/frog/leg/ssa/co...
dart/frog/leg/ssa/codegen.dart:801: if (node.isConst) {
On 2012/02/24 10:15:09, floitsch wrote:
> You can keep this code (as otherwise you can't test this patch), but I will
> remove it with my CTC changes.
> const Arrays must go through the CTC as otherwise canonicalization is wrong.

Done.

https://chromiumcodereview.appspot.com/9453021/diff/1001/dart/frog/leg/ssa/no...
File dart/frog/leg/ssa/nodes.dart (right):

https://chromiumcodereview.appspot.com/9453021/diff/1001/dart/frog/leg/ssa/no...
dart/frog/leg/ssa/nodes.dart:2123: HLiteralList(inputs, this.isConst) :
super(inputs);
On 2012/02/24 13:24:33, ngeoffray wrote:
> Please add a TODO(floitsch): remove 'isConst' once the CTC handles it.

Done.

https://chromiumcodereview.appspot.com/9453021/diff/1001/dart/tests/corelib/c...
File dart/tests/corelib/corelib-leg.status (right):

https://chromiumcodereview.appspot.com/9453021/diff/1001/dart/tests/corelib/c...
dart/tests/corelib/corelib-leg.status:43: RegExpAllMatchesTest: Fail, OK #
NoSuchMethodException : method not found: 'forEach'
On 2012/02/24 13:24:33, ngeoffray wrote:
> Why is that OK now?

The comment above explains why. The test is broken.

[ahe, 2012-02-27 12:35:05]

Merged with latest changes and added code to ensure const lists are immutable
and non-growable.