Add List.insert.

Add List.insert.

BUG= http://dartbug.com/5375

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

[srdjan, 2013-03-04 00:53:47]

https://codereview.chromium.org/12383073/diff/1/runtime/lib/array.dart
File runtime/lib/array.dart (right):

https://codereview.chromium.org/12383073/diff/1/runtime/lib/array.dart#newcod...
runtime/lib/array.dart:271: "Cannot add to a non-extendable array");
to an immutable array

https://codereview.chromium.org/12383073/diff/1/runtime/lib/growable_array.dart
File runtime/lib/growable_array.dart (right):

https://codereview.chromium.org/12383073/diff/1/runtime/lib/growable_array.da...
runtime/lib/growable_array.dart:12: if (index is! int) throw new
ArgumentError(index);
Per Lars's instructions no explicit type checks should be added. Please remove
line above.

https://codereview.chromium.org/12383073/diff/1/runtime/lib/growable_array.da...
runtime/lib/growable_array.dart:15: this.length = newLength;
Why not just 'this.length+= 1;'?

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

https://codereview.chromium.org/12383073/diff/1/sdk/lib/core/list.dart#newcod...
sdk/lib/core/list.dart:197: * Throws an [ArgumentError] if [index] is not an
[int].
That is not correct for checked mode.

https://codereview.chromium.org/12383073/diff/1/sdk/lib/core/list.dart#newcod...
sdk/lib/core/list.dart:198: * Throws an [RangeError] if the [index] does not
point inside
s/an/a/

[Lasse Reichstein Nielsen, 2013-03-04 09:06:02]

lgtm

https://codereview.chromium.org/12383073/diff/1/runtime/lib/array.dart
File runtime/lib/array.dart (right):

https://codereview.chromium.org/12383073/diff/1/runtime/lib/array.dart#newcod...
runtime/lib/array.dart:271: "Cannot add to a non-extendable array");
Consider changing "immutable" to "unmodifiable" while you are here.

https://codereview.chromium.org/12383073/diff/1/runtime/lib/growable_array.dart
File runtime/lib/growable_array.dart (right):

https://codereview.chromium.org/12383073/diff/1/runtime/lib/growable_array.da...
runtime/lib/growable_array.dart:13: if (index < 0 || index > length) throw new
RangeError(index);
Consider special-casing index == length into 'add'.

https://codereview.chromium.org/12383073/diff/1/runtime/lib/growable_array.da...
runtime/lib/growable_array.dart:15: this.length = newLength;
Remember oldLength instead. It seems it's what you need below. Or just remember
the number of elements after the index.

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

https://codereview.chromium.org/12383073/diff/1/sdk/lib/core/list.dart#newcod...
sdk/lib/core/list.dart:197: * Throws an [ArgumentError] if [index] is not an
[int].
I really prefer to just say "It's an error if ..." and then throw whatever Error
I think fits.
You don't need to know the exact error, and add special code to avoid throwing a
different error.
In this case, requiring a special error in case for something that is already a
bad type is overspecifying. Ofcourse it's an error if the "int" argument is not
assignable to int. No need to say that. You can say that "index" must not be
null.

https://codereview.chromium.org/12383073/diff/1/sdk/lib/core/list.dart#newcod...
sdk/lib/core/list.dart:200: * Throws an [UnsupportedError], and doesn't remove
the element,
No need to say that. That's what throwing an UnsupportedError means: I don't
support that operation. We already stated in the List documentation that
fixed-length lists don't support operations that changes the length.

[Sean Eagan, 2013-03-04 16:37:27]

On 2013/03/02 16:50:43, floitsch wrote:

Naming suggestion:

I think "insertAt" is redundant, it's impossible to insert without it being at a
certain index.  Wouldn't just "insert" be sufficient?  Another option would be
"addAt" to match "removeAt" and avoid adding a new term.

Also, an "insertAll" or "addAllAt" would be useful along with this.

[floitsch, 2013-03-05 17:51:57]

I was really tempted to switch to "insert", but "insertAt" still sounds more
right.
"addAt" doesn't really work for me.
add with an optional named argument "at" would make sense, but I don't want to
abuse named arguments either.
Currently sticking to insertAt, but not committing yet, so we can discuss this
more.

https://codereview.chromium.org/12383073/diff/1/runtime/lib/array.dart
File runtime/lib/array.dart (right):

https://codereview.chromium.org/12383073/diff/1/runtime/lib/array.dart#newcod...
runtime/lib/array.dart:271: "Cannot add to a non-extendable array");
On 2013/03/04 00:53:47, srdjan wrote:
> to an immutable array

Done.

https://codereview.chromium.org/12383073/diff/1/runtime/lib/array.dart#newcod...
runtime/lib/array.dart:271: "Cannot add to a non-extendable array");
On 2013/03/04 09:06:02, Lasse Reichstein Nielsen wrote:
> Consider changing "immutable" to "unmodifiable" while you are here.

not in this CL. Filed http://dartbug.com/8925

https://codereview.chromium.org/12383073/diff/1/runtime/lib/growable_array.dart
File runtime/lib/growable_array.dart (right):

https://codereview.chromium.org/12383073/diff/1/runtime/lib/growable_array.da...
runtime/lib/growable_array.dart:12: if (index is! int) throw new
ArgumentError(index);
On 2013/03/04 00:53:47, srdjan wrote:
> Per Lars's instructions no explicit type checks should be added. Please remove
> line above.

Done.

https://codereview.chromium.org/12383073/diff/1/runtime/lib/growable_array.da...
runtime/lib/growable_array.dart:13: if (index < 0 || index > length) throw new
RangeError(index);
On 2013/03/04 09:06:02, Lasse Reichstein Nielsen wrote:
> Consider special-casing index == length into 'add'.

Done.

https://codereview.chromium.org/12383073/diff/1/runtime/lib/growable_array.da...
runtime/lib/growable_array.dart:15: this.length = newLength;
On 2013/03/04 09:06:02, Lasse Reichstein Nielsen wrote:
> Remember oldLength instead. It seems it's what you need below. Or just
remember
> the number of elements after the index.

Done.

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

https://codereview.chromium.org/12383073/diff/1/sdk/lib/core/list.dart#newcod...
sdk/lib/core/list.dart:197: * Throws an [ArgumentError] if [index] is not an
[int].
On 2013/03/04 09:06:02, Lasse Reichstein Nielsen wrote:
> I really prefer to just say "It's an error if ..." and then throw whatever
Error
> I think fits.
> You don't need to know the exact error, and add special code to avoid throwing
a
> different error.
> In this case, requiring a special error in case for something that is already
a
> bad type is overspecifying. Ofcourse it's an error if the "int" argument is
not
> assignable to int. No need to say that. You can say that "index" must not be
> null.

The documentation had been copied from removeAt.
done.

[Sean Eagan, 2013-03-05 20:32:27]

On 2013/03/05 17:51:57, floitsch wrote:
> I was really tempted to switch to "insert", but "insertAt" still sounds more
> right.
> Currently sticking to insertAt, but not committing yet, so we can discuss this
> more.

I couldn't find any languages with "insertAt", but I found C#, ruby, python, and
C++ with "insert":

http://msdn.microsoft.com/en-us/library/sey5k5z4.aspx
http://www.ruby-doc.org/core-2.0/Array.html#method-i-insert
http://www.tutorialspoint.com/python/list_insert.htm
http://www.cplusplus.com/reference/list/list/insert/

Also, I filed a bug to replace List.insertRange with a List.insertAll at
http://dartbug.com/8924

[floitsch, 2013-03-06 15:35:51]

Ok. that convinced at least me.
@Lasse: PTAL.

[srdjan, 2013-03-06 17:08:37]

LGTM

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

https://codereview.chromium.org/12383073/diff/13001/sdk/lib/core/list.dart#ne...
sdk/lib/core/list.dart:198: * up by one position.
'up' is an interesting direction. I always though of lists in terms of left and
right :-).

[Lasse Reichstein Nielsen, 2013-03-07 09:57:53]

LGTM.

https://codereview.chromium.org/12383073/diff/13001/editor/util/plugins/com.g...
File editor/util/plugins/com.google.dart.java2dart/resources/java_core.dart
(right):

https://codereview.chromium.org/12383073/diff/13001/editor/util/plugins/com.g...
editor/util/plugins/com.google.dart.java2dart/resources/java_core.dart:293: void
insert(int index, E value) {
It's a little inconsistent with removeAt, but I guess it can't be avoided
without overloading.

If we had slice-assignment, it would just be:
  list[index:index] = [value]
We should have slice assignment!

https://codereview.chromium.org/12383073/diff/13001/runtime/lib/growable_arra...
File runtime/lib/growable_array.dart (right):

https://codereview.chromium.org/12383073/diff/13001/runtime/lib/growable_arra...
runtime/lib/growable_array.dart:12: if (index < 0 || index > length) throw new
RangeError(index);
new RangeError.range(index, 0, length)

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

https://codereview.chromium.org/12383073/diff/13001/sdk/lib/core/list.dart#ne...
sdk/lib/core/list.dart:198: * up by one position.
I'd expect "moving up" to move towards the start.
I can see this is consistent with removeAt, which says "moves ... down by one
position". Not too happy about that either (even though I wrote it).

I'd prefer switching up/down, or switching to ... something else. "forward/back"
won't work, it's just as unobvious which direction is which. "away from the
start of the list"/"towards the start of the list"? Not great.
Shifts elements to the right/left?

https://codereview.chromium.org/12383073/diff/13001/sdk/lib/core/list.dart#ne...
sdk/lib/core/list.dart:200: * It is an error if the [index] is `null`, or if the
[index] does not point
Skip the null part. That's included in "not pointing inside the list".

https://codereview.chromium.org/12383073/diff/13001/tools/dom/templates/html/...
File tools/dom/templates/html/impl/impl_Element.darttemplate (right):

https://codereview.chromium.org/12383073/diff/13001/tools/dom/templates/html/...
tools/dom/templates/html/impl/impl_Element.darttemplate:222: throw new
UnimplementedError("insert on ElementLists");
Can you use:
 _element.$dom_insertBefore(element, _childElements[index]);
?

https://codereview.chromium.org/12383073/diff/13001/tools/dom/templates/html/...
File tools/dom/templates/html/impl/impl_Node.darttemplate (right):

https://codereview.chromium.org/12383073/diff/13001/tools/dom/templates/html/...
tools/dom/templates/html/impl/impl_Node.darttemplate:91: throw new
UnimplementedError("insert on NodeLists");
And
 _this.$dom_insertBefore(node, _this.$dom_childNodes[index]);

[floitsch, 2013-03-07 12:53:53]

https://codereview.chromium.org/12383073/diff/13001/editor/util/plugins/com.g...
File editor/util/plugins/com.google.dart.java2dart/resources/java_core.dart
(right):

https://codereview.chromium.org/12383073/diff/13001/editor/util/plugins/com.g...
editor/util/plugins/com.google.dart.java2dart/resources/java_core.dart:293: void
insert(int index, E value) {
On 2013/03/07 09:57:53, Lasse Reichstein Nielsen wrote:
> It's a little inconsistent with removeAt, but I guess it can't be avoided
> without overloading.
> 
> If we had slice-assignment, it would just be:
>   list[index:index] = [value]
> We should have slice assignment!

I looked at the other languages (the ones Sean linked to), and I saw some that
had the same API. So I'm ok with it.

https://codereview.chromium.org/12383073/diff/13001/runtime/lib/growable_arra...
File runtime/lib/growable_array.dart (right):

https://codereview.chromium.org/12383073/diff/13001/runtime/lib/growable_arra...
runtime/lib/growable_array.dart:12: if (index < 0 || index > length) throw new
RangeError(index);
On 2013/03/07 09:57:53, Lasse Reichstein Nielsen wrote:
> new RangeError.range(index, 0, length)

Done.

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

https://codereview.chromium.org/12383073/diff/13001/sdk/lib/core/list.dart#ne...
sdk/lib/core/list.dart:198: * up by one position.
On 2013/03/07 09:57:53, Lasse Reichstein Nielsen wrote:
> I'd expect "moving up" to move towards the start.
> I can see this is consistent with removeAt, which says "moves ... down by one
> position". Not too happy about that either (even though I wrote it).
> 
> I'd prefer switching up/down, or switching to ... something else.
"forward/back"
> won't work, it's just as unobvious which direction is which. "away from the
> start of the list"/"towards the start of the list"? Not great.
> Shifts elements to the right/left? 

Changed to "shifts all later elements towards the end of the list.

https://codereview.chromium.org/12383073/diff/13001/sdk/lib/core/list.dart#ne...
sdk/lib/core/list.dart:200: * It is an error if the [index] is `null`, or if the
[index] does not point
On 2013/03/07 09:57:53, Lasse Reichstein Nielsen wrote:
> Skip the null part. That's included in "not pointing inside the list".

Done.

https://codereview.chromium.org/12383073/diff/13001/tools/dom/templates/html/...
File tools/dom/templates/html/impl/impl_Element.darttemplate (right):

https://codereview.chromium.org/12383073/diff/13001/tools/dom/templates/html/...
tools/dom/templates/html/impl/impl_Element.darttemplate:222: throw new
UnimplementedError("insert on ElementLists");
On 2013/03/07 09:57:53, Lasse Reichstein Nielsen wrote:
> Can you use:
>  _element.$dom_insertBefore(element, _childElements[index]);
> ?

No. doesn't exist in Dart yet. That's why I added Pete as reviewer.

https://codereview.chromium.org/12383073/diff/13001/tools/dom/templates/html/...
File tools/dom/templates/html/impl/impl_Node.darttemplate (right):

https://codereview.chromium.org/12383073/diff/13001/tools/dom/templates/html/...
tools/dom/templates/html/impl/impl_Node.darttemplate:91: throw new
UnimplementedError("insert on NodeLists");
On 2013/03/07 09:57:53, Lasse Reichstein Nielsen wrote:
> And
>  _this.$dom_insertBefore(node, _this.$dom_childNodes[index]);

There is no 'dom_insertBefore' in Dart.

[blois, 2013-03-08 02:32:40]

Sorry about delay, hope I wasn't holding anything up.

insertBefore should be able to be used for the node list, not sure it's a good
fit for the element list. But I've been doing some optimizations in this area
and would be happy to fix these up after this CL is in.

https://codereview.chromium.org/12383073/diff/13001/tools/dom/templates/html/...
File tools/dom/templates/html/impl/impl_Element.darttemplate (right):

https://codereview.chromium.org/12383073/diff/13001/tools/dom/templates/html/...
tools/dom/templates/html/impl/impl_Element.darttemplate:222: throw new
UnimplementedError("insert on ElementLists");
On 2013/03/07 12:53:53, floitsch wrote:
> On 2013/03/07 09:57:53, Lasse Reichstein Nielsen wrote:
> > Can you use:
> >  _element.$dom_insertBefore(element, _childElements[index]);
> > ?
> 
> No. doesn't exist in Dart yet. That's why I added Pete as reviewer.

I would leave this as unsupported for now. I can investigate  implementing.
insertBefore doesn't quite fit as it deals with nodes which are a superset of
elements.

https://codereview.chromium.org/12383073/diff/13001/tools/dom/templates/html/...
File tools/dom/templates/html/impl/impl_Node.darttemplate (right):

https://codereview.chromium.org/12383073/diff/13001/tools/dom/templates/html/...
tools/dom/templates/html/impl/impl_Node.darttemplate:91: throw new
UnimplementedError("insert on NodeLists");
On 2013/03/07 12:53:53, floitsch wrote:
> On 2013/03/07 09:57:53, Lasse Reichstein Nielsen wrote:
> > And
> >  _this.$dom_insertBefore(node, _this.$dom_childNodes[index]);
> 
> There is no 'dom_insertBefore' in Dart.

It's just insertBefore, on node.

[sra1, 2013-03-08 18:56:52]

https://chromiumcodereview.appspot.com/12383073/diff/1/tools/dom/templates/ht...
File tools/dom/templates/html/impl/impl_Element.darttemplate (right):

https://chromiumcodereview.appspot.com/12383073/diff/1/tools/dom/templates/ht...
tools/dom/templates/html/impl/impl_Element.darttemplate:222: throw new
UnimplementedError("insertAt on ElementLists");
insertBefore the current element at that slot?

[floitsch, 2013-03-08 23:17:30]

PTAL.
@blois: I have added the insertBefore, but I'm pretty sure it is currently
untested.
@srdjan: I had to add an is-check. (growable_array.dart)

https://codereview.chromium.org/12383073/diff/1/tools/dom/templates/html/impl...
File tools/dom/templates/html/impl/impl_Element.darttemplate (right):

https://codereview.chromium.org/12383073/diff/1/tools/dom/templates/html/impl...
tools/dom/templates/html/impl/impl_Element.darttemplate:222: throw new
UnimplementedError("insertAt on ElementLists");
On 2013/03/08 18:56:53, sra1 wrote:
> insertBefore the current element at that slot?

Pete told me to keep it unimplemented: "insertBefore doesn't quite fit as it
deals with nodes which are a superset of
elements".

[floitsch, 2013-03-14 15:56:07]

ping.

[blois, 2013-03-14 16:07:18]

lgtm

[floitsch, 2013-03-14 17:39:14]

Committed patchset #5 manually as r20037 (presubmit successful).