Refactor List.setRange function.

Refactor List.setRange function.

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

[floitsch, 2013-04-10 23:27:49]

Note: I'm still missing a bug for native setRange functions (typed lists) that
probably don't check the arguments the correct way.

[Lasse Reichstein Nielsen, 2013-04-11 06:11:55]

I've looked through the uses of setRange in this CL, and I think changing to
  setList(int at, List source, [int start = 0, int end])
(or putList, or something else) would improve most of them, and not hurt the
rest.
In most cases, the second argument goes away, because the default start
and end will be exactly what you want.
In some cases, it will be moved to the end instead, but often in a simpler
form - instead of setRange(i, i + x, data, o) it becomes 
setList(i, data, o, x).
The only cases that would require more arguments is
  setRange(a, a + l, list)
where l < list.length, because that would become
  setList(a, list, 0, l)
and I can live with that.

I actually find it more understandable this way. If I ask for
  x.setRange(a, a + l, list)
it's not obvious what happens if list is shorter than b-a.
If I say
  x.setList(a, list, 0, l)
it's more visible that 0..l must be a range of list, and that this
range is what needs to fit in x at offset a.

It's still an error in setList(at, list, start, end) if:
  start < 0,
  end < start
  end > list.length
  at < 0
  at + (end - start) > list.length

Also see further arguments in inline comment.

https://codereview.chromium.org/13863012/diff/6001/runtime/bin/process_patch....
File runtime/bin/process_patch.dart (right):

https://codereview.chromium.org/13863012/diff/6001/runtime/bin/process_patch....
runtime/bin/process_patch.dart:202: exitDataBuffer.setRange(exitDataRead,
exitDataRead + data.length, data);
I still think it reads better as
  .setList(exitDataRead, data)
i.e., with signature
  .setList(int at, List source, [int start = 0, int end])
It won't be a "range" function on the destination, but it is acting on a range
of the source.

It avoids the common second argument of setRange(i, i + x.length, x).
As an extra bonus, it changes the signature to be incompatible with current
uses, instead of just changing the meaning of the second integer (that's
undetected bugs waiting to happen)

This is the low-level, efficient list copy. As such, it is in a sense not at the
same level of abstraction as the other "Range functions", and it would be more
obvious with a non-range name.

https://codereview.chromium.org/13863012/diff/6001/sdk/lib/io/secure_socket.dart
File sdk/lib/io/secure_socket.dart (right):

https://codereview.chromium.org/13863012/diff/6001/sdk/lib/io/secure_socket.d...
sdk/lib/io/secure_socket.dart:465: int write(List<int> data, [int offset, int
bytes]) {
This code doesn't seem to handle the case where offset and bytes are null
because they were not passed, even though they are optional. I'd expect offset
to default to zero, and bytes  to data.length - offset.

[Sean Eagan, 2013-04-11 14:22:07]

https://chromiumcodereview.appspot.com/13863012/diff/6001/sdk/lib/core/list.dart
File sdk/lib/core/list.dart (left):

https://chromiumcodereview.appspot.com/13863012/diff/6001/sdk/lib/core/list.d...
sdk/lib/core/list.dart:253: 

I think there is a lot to be learned from C#'s setRange here:

http://msdn.microsoft.com/en-us/library/system.collections.arraylist.setrange...

* The "end" parameter is redundant and thus error prone.  It should already be
deducible by "start" + "from.length".  When  you don't want all of "from", just
call "getRange", "skip", or "take" on it.  Same goes for "startFrom"

* "from" should be generalized to Iterable.


I would also consider:

* for non-fixed-length Lists, should it be allowed to set beyond the current
length?

* should "start" be optional and default to 0 to facilitate overwriting the
entire List?

* rename to "setAll" since the arguments are similar to "addAll", "removeAll",
and the hypothetical "insertAll" (http://dartbug.com/8924).


So something like:

void setAll(Iterable<E> all, [int start = 0]);

Usage:

list.setAll(otherList);
list.setAll(iterable.take(10), start);
list.setAll(otherList.getRange(10, 20), start);


Thanks!

[Lasse Reichstein Nielsen, 2013-04-11 14:33:51]

https://chromiumcodereview.appspot.com/13863012/diff/6001/sdk/lib/core/list.dart
File sdk/lib/core/list.dart (left):

https://chromiumcodereview.appspot.com/13863012/diff/6001/sdk/lib/core/list.d...
sdk/lib/core/list.dart:253: 
I generally agree with Sean.
If the is not the efficient low-level operation, then we might as well go full
Iterable on it and do:
  setSomething(Iterable source, [int at])
About growing the array if the source doesn't fit, I don't think it's a good
idea. There can be useful cases for writing up to the end, and throwing at that
point (we can't really do it earlier for a fixed-length list) would exactly give
this functionality to someone willing to catch-and-continue. Might as well make
it directly available.
So, perhaps better:
  int setSomething(Iterable source, [int at = 0])
that returns the number of elements written (or the index after the last one
written).
Then you can do:
  while (index < list.length) {
    index += list.setSomething(source, index);
  }
to fill the rest of a list with repeated content of the iterable.


(With that in mind, would replaceRange be better as:
   replaceRange(Iterable source, [int start = 0, int end])
so you can default to replacing everything?)

[floitsch, 2013-04-11 15:54:06]

On 2013/04/11 14:33:51, Lasse Reichstein Nielsen wrote:
>
https://chromiumcodereview.appspot.com/13863012/diff/6001/sdk/lib/core/list.dart
> File sdk/lib/core/list.dart (left):
> 
>
https://chromiumcodereview.appspot.com/13863012/diff/6001/sdk/lib/core/list.d...
> sdk/lib/core/list.dart:253: 
> I generally agree with Sean.
> If the is not the efficient low-level operation, then we might as well go full
> Iterable on it and do:
>   setSomething(Iterable source, [int at])
> About growing the array if the source doesn't fit, I don't think it's a good
> idea. There can be useful cases for writing up to the end, and throwing at
that
> point (we can't really do it earlier for a fixed-length list) would exactly
give
> this functionality to someone willing to catch-and-continue. Might as well
make
> it directly available.
> So, perhaps better:
>   int setSomething(Iterable source, [int at = 0])
> that returns the number of elements written (or the index after the last one
> written).
> Then you can do:
>   while (index < list.length) {
>     index += list.setSomething(source, index);
>   }
> to fill the rest of a list with repeated content of the iterable.
> 
> 
> (With that in mind, would replaceRange be better as:
>    replaceRange(Iterable source, [int start = 0, int end])
> so you can default to replacing everything?)

Having a default argument so that you don't have to write "0" is not a good
idea. It is furthermore after the source, which makes it look like it is part of
that one.
So no: the at-position should not be optional.

[Sean Eagan, 2013-04-11 20:07:19]

On 2013/04/11 15:54:06, floitsch wrote:
> Having a default argument so that you don't have to write "0" is not a good
idea. 

List.indexOf does this

> It is furthermore after the source, which makes it look like it is part of
that one.

I can see that

[floitsch, 2013-04-12 20:57:02]

PTAL.
I'm pretty happy with the signatures now.

[floitsch, 2013-04-12 23:35:42]

adding Soeren for the change to secure_socket.dart.

https://codereview.chromium.org/13863012/diff/6001/sdk/lib/io/secure_socket.dart
File sdk/lib/io/secure_socket.dart (right):

https://codereview.chromium.org/13863012/diff/6001/sdk/lib/io/secure_socket.d...
sdk/lib/io/secure_socket.dart:465: int write(List<int> data, [int offset, int
bytes]) {
On 2013/04/11 06:11:55, Lasse Reichstein Nielsen wrote:
> This code doesn't seem to handle the case where offset and bytes are null
> because they were not passed, even though they are optional. I'd expect offset
> to default to zero, and bytes  to data.length - offset.

Done.

[Søren Gjesse, 2013-04-15 07:05:17]

lgtm for secure_socket.dart

[floitsch, 2013-04-15 09:17:58]

Reassigning to Anders for review.

[Anders Johnsen, 2013-04-15 09:24:04]

LGTM file issue filed.

https://chromiumcodereview.appspot.com/13863012/diff/21001/sdk/lib/core/list....
File sdk/lib/core/list.dart (right):

https://chromiumcodereview.appspot.com/13863012/diff/21001/sdk/lib/core/list....
sdk/lib/core/list.dart:266: * Copies the elements of [iterable], skipping the
[skipCount] first elements
first elements,

https://chromiumcodereview.appspot.com/13863012/diff/21001/sdk/lib/core/list....
sdk/lib/core/list.dart:276: * skipping [skipCount] elements.
Will it throw when at end, or before it starts copying?

https://chromiumcodereview.appspot.com/13863012/diff/21001/sdk/lib/io/secure_...
File sdk/lib/io/secure_socket.dart (right):

https://chromiumcodereview.appspot.com/13863012/diff/21001/sdk/lib/io/secure_...
sdk/lib/io/secure_socket.dart:473: if (offset == null) offset = 0;
Why was this introduced?

https://chromiumcodereview.appspot.com/13863012/diff/21001/tests/co19/co19-da...
File tests/co19/co19-dart2js.status (right):

https://chromiumcodereview.appspot.com/13863012/diff/21001/tests/co19/co19-da...
tests/co19/co19-dart2js.status:503: LibTest/core/Iterable/any_A01_t04: Fail #
setRange now takes end-argument. Issue TODO
File issue.

[Lasse Reichstein Nielsen, 2013-04-15 10:16:10]

Still don't like signature!

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

https://codereview.chromium.org/13863012/diff/21001/sdk/lib/core/list.dart#ne...
sdk/lib/core/list.dart:276: * skipping [skipCount] elements.
It must be able to throw at the end, because there is no other way to know that
it doesn't have enough elements in the general case.
So it might as well just throw there in all cases.

Which means that it's possible to use this function to write "as many elements
as in X" at position "i" by doing:
  try {
    foo.setRange(i, foo.length, X)
  } catch (e) {}
We do not want to make this the only way to get that functionality. That is why
I prefer:
  List.setFromIterable(int at, Iterable source)
or even 
  List.setRange(int at, Iterable source, [int start = 0, int end])
but you can do that with skip/take on the iterable.

This signature is counter productive and confusing (but I think I already
mentioned that in another CL). The "end" is not related to anything here. It is
the wrong place to put the limit, because it might not match the length of the
iterable, so it would make it more direct to put it as a limit on the iterable
instead. And accept shorter iterables.

https://codereview.chromium.org/13863012/diff/21001/sdk/lib/core/list.dart#ne...
sdk/lib/core/list.dart:278: void setRange(int start, int end, Iterable<E>
iterable, [int skipCount = 0]);
If this is working on an Iterable, and not the efficient sublist-copy function,
I don't mind dropping the "skipCount" parameter. You can always use skip on the
iterable.

I still don't like the "start, end of this" way of making a requirement on
"iterable". Put the limits on iterable directly.

[floitsch, 2013-04-15 13:45:34]

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

https://codereview.chromium.org/13863012/diff/21001/sdk/lib/core/list.dart#ne...
sdk/lib/core/list.dart:266: * Copies the elements of [iterable], skipping the
[skipCount] first elements
On 2013/04/15 09:24:04, Anders Johnsen wrote:
> first elements,

Done.

https://codereview.chromium.org/13863012/diff/21001/sdk/lib/core/list.dart#ne...
sdk/lib/core/list.dart:278: void setRange(int start, int end, Iterable<E>
iterable, [int skipCount = 0]);
On 2013/04/15 10:16:10, Lasse Reichstein Nielsen wrote:
> If this is working on an Iterable, and not the efficient sublist-copy
function,
> I don't mind dropping the "skipCount" parameter. You can always use skip on
the
> iterable.
> 
> I still don't like the "start, end of this" way of making a requirement on
> "iterable". Put the limits on iterable directly.

This is the efficient method. Keeping 'skipCount'.

https://codereview.chromium.org/13863012/diff/21001/sdk/lib/io/secure_socket....
File sdk/lib/io/secure_socket.dart (right):

https://codereview.chromium.org/13863012/diff/21001/sdk/lib/io/secure_socket....
sdk/lib/io/secure_socket.dart:473: if (offset == null) offset = 0;
On 2013/04/15 09:24:04, Anders Johnsen wrote:
> Why was this introduced?

I don't think it worked before.

https://codereview.chromium.org/13863012/diff/21001/tests/co19/co19-dart2js.s...
File tests/co19/co19-dart2js.status (right):

https://codereview.chromium.org/13863012/diff/21001/tests/co19/co19-dart2js.s...
tests/co19/co19-dart2js.status:503: LibTest/core/Iterable/any_A01_t04: Fail #
setRange now takes end-argument. Issue TODO
On 2013/04/15 09:24:04, Anders Johnsen wrote:
> File issue.

Done.

[floitsch, 2013-04-15 13:53:14]

Committed patchset #8 manually as r21452 (presubmit successful).