Native implementation of List,setRange for frog.

Implementation of List.setRange for frog. Re-enable test.

BUG=934
Committed: https://code.google.com/p/dart/source/detail?r=3043

[jimhug, 2012-01-04 17:38:17]

Thanks for fixing your own bug report!  This should be good with the change
suggested below - assuming that you've run the current test suite? 

I would have hoped that this change would have fixed the current failures on
tests/corelib/ListSetRangeTest.dart.  Could you run that test and take a look at
why its still failing with your change?

Thanks - Jim

http://codereview.chromium.org/8983019/diff/1/frog/lib/corelib_impl.dart
File frog/lib/corelib_impl.dart (right):

http://codereview.chromium.org/8983019/diff/1/frog/lib/corelib_impl.dart#newc...
frog/lib/corelib_impl.dart:77: startFrom = (startFrom === null ? 0 : startFrom);
startFrom == null will do the correct comparison on the JS side to handle either
null or undefined - which is something that the dart compiler will occasionally
rely on.

[dominich, 2012-01-04 17:56:09]

$ ./tools/test.py --arch=ia32 --component=frog corelib/ListSetRangeTest
[00:15|% 100|+   1|-   0]: Done 

Seems to pass now, unless I messed up the invocation.

[dominich, 2012-01-04 20:22:48]

Even in checked mode, the test still failed without the explicit bounds
checking.

A better way to balance correctness vs performance might be to have a Checked
version of the code that contains the checks and calls through to the Unchecked
version. Then unchecked Dart code can use the light-weight unchecked version
directly and have no overhead from bounds checking.

[Jennifer Messerly, 2012-01-05 00:44:47]

On 2012/01/04 20:22:48, dominich wrote:
> Even in checked mode, the test still failed without the explicit bounds
> checking.
> 
> A better way to balance correctness vs performance might be to have a Checked
> version of the code that contains the checks and calls through to the
Unchecked
> version. Then unchecked Dart code can use the light-weight unchecked version
> directly and have no overhead from bounds checking.

Yeah, I think []= bounds checks in are not actually implemented yet :)

but other than small style suggestion LGTM

[dominich, 2012-01-05 15:33:00]

On 2012/01/05 00:44:47, John Messerly wrote:
> On 2012/01/04 20:22:48, dominich wrote:
> > Even in checked mode, the test still failed without the explicit bounds
> > checking.
> > 
> > A better way to balance correctness vs performance might be to have a
Checked
> > version of the code that contains the checks and calls through to the
> Unchecked
> > version. Then unchecked Dart code can use the light-weight unchecked version
> > directly and have no overhead from bounds checking.
> 
> Yeah, I think []= bounds checks in are not actually implemented yet :)
> 
> but other than small style suggestion LGTM

I might be missing a comment from you - I don't see a style suggestion.

[Jennifer Messerly, 2012-01-05 17:59:33]

oops! somehow I sent a reply in the CR tool but it didn't get published. Sorry.

http://codereview.chromium.org/8983019/diff/5001/frog/lib/corelib_impl.dart
File frog/lib/corelib_impl.dart (right):

http://codereview.chromium.org/8983019/diff/5001/frog/lib/corelib_impl.dart#n...
frog/lib/corelib_impl.dart:77: if (length == 0)
style nit: we generally put these on one line, or else add { }

[dominich, 2012-01-05 18:19:57]

http://codereview.chromium.org/8983019/diff/1/frog/lib/corelib_impl.dart
File frog/lib/corelib_impl.dart (right):

http://codereview.chromium.org/8983019/diff/1/frog/lib/corelib_impl.dart#newc...
frog/lib/corelib_impl.dart:77: startFrom = (startFrom === null ? 0 : startFrom);
On 2012/01/04 17:38:17, jimhug wrote:
> startFrom == null will do the correct comparison on the JS side to handle
either
> null or undefined - which is something that the dart compiler will
occasionally
> rely on.

Done.

http://codereview.chromium.org/8983019/diff/5001/frog/lib/corelib_impl.dart
File frog/lib/corelib_impl.dart (right):

http://codereview.chromium.org/8983019/diff/5001/frog/lib/corelib_impl.dart#n...
frog/lib/corelib_impl.dart:77: if (length == 0)
On 2012/01/05 17:59:33, John Messerly wrote:
> style nit: we generally put these on one line, or else add { }

Done.

[sra1, 2012-01-05 18:38:44]

http://codereview.chromium.org/8983019/diff/9001/frog/lib/corelib_impl.dart
File frog/lib/corelib_impl.dart (right):

http://codereview.chromium.org/8983019/diff/9001/frog/lib/corelib_impl.dart#n...
frog/lib/corelib_impl.dart:75: void setRange(int start, int length, List<E>
from, [int startFrom]) {
Can you do  [int startFrom = 0] ?

http://codereview.chromium.org/8983019/diff/9001/frog/lib/corelib_impl.dart#n...
frog/lib/corelib_impl.dart:85: startFrom = (startFrom == null ? 0 : startFrom);
This test would be unnecessary with the correct default value

[dominich, 2012-01-05 18:53:21]

Thanks for the default value hint; I completely forgot about that feature.

http://codereview.chromium.org/8983019/diff/9001/frog/lib/corelib_impl.dart
File frog/lib/corelib_impl.dart (right):

http://codereview.chromium.org/8983019/diff/9001/frog/lib/corelib_impl.dart#n...
frog/lib/corelib_impl.dart:75: void setRange(int start, int length, List<E>
from, [int startFrom]) {
On 2012/01/05 18:38:44, sra1 wrote:
> Can you do  [int startFrom = 0] ?

Done.

http://codereview.chromium.org/8983019/diff/9001/frog/lib/corelib_impl.dart#n...
frog/lib/corelib_impl.dart:85: startFrom = (startFrom == null ? 0 : startFrom);
On 2012/01/05 18:38:44, sra1 wrote:
> This test would be unnecessary with the correct default value

Done.

[Jennifer Messerly, 2012-01-05 18:55:39]

On 2012/01/05 18:53:21, dominich wrote:
> Thanks for the default value hint; I completely forgot about that feature.
> 
> http://codereview.chromium.org/8983019/diff/9001/frog/lib/corelib_impl.dart
> File frog/lib/corelib_impl.dart (right):
> 
>
http://codereview.chromium.org/8983019/diff/9001/frog/lib/corelib_impl.dart#n...
> frog/lib/corelib_impl.dart:75: void setRange(int start, int length, List<E>
> from, [int startFrom]) {
> On 2012/01/05 18:38:44, sra1 wrote:
> > Can you do  [int startFrom = 0] ?
> 
> Done.
> 
>
http://codereview.chromium.org/8983019/diff/9001/frog/lib/corelib_impl.dart#n...
> frog/lib/corelib_impl.dart:85: startFrom = (startFrom == null ? 0 :
startFrom);
> On 2012/01/05 18:38:44, sra1 wrote:
> > This test would be unnecessary with the correct default value
> 
> Done.

lgtm :)

[dominich, 2012-01-05 18:57:06]

Great! I don't think I have commit rights for Dart so could someone do the
honours?