Fix missing concurrency check in [].forEach.

Fix missing concurrency check in [].forEach.

Also fix a bad range check for List.setRange.

Added more tests.

BUG=http://dartbug.com/10045

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

[Søren Gjesse, 2013-04-24 09:00:49]

lgtm

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

https://codereview.chromium.org/14468004/diff/1/runtime/lib/typeddata.dart#ne...
runtime/lib/typeddata.dart:463: 
Is it documented in toList that the default is a growable list?

https://codereview.chromium.org/14468004/diff/1/tests/corelib/list_test.dart
File tests/corelib/list_test.dart (right):

https://codereview.chromium.org/14468004/diff/1/tests/corelib/list_test.dart#...
tests/corelib/list_test.dart:416: testSafeConcurrentModification(() {
This doesn't change length, but the result of forEach is affected. I assume this
is by design.

Is it documented under which circumstances a "concurrent modification error"
will happen (as some mutations are actually allowed)?

[Lasse Reichstein Nielsen, 2013-04-24 09:30:57]

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

https://codereview.chromium.org/14468004/diff/1/runtime/lib/typeddata.dart#ne...
runtime/lib/typeddata.dart:463: 
Yes. It's defined the same way on Iterable, so the default value must be the
same here.

https://codereview.chromium.org/14468004/diff/1/tests/corelib/list_test.dart
File tests/corelib/list_test.dart (right):

https://codereview.chromium.org/14468004/diff/1/tests/corelib/list_test.dart#...
tests/corelib/list_test.dart:416: testSafeConcurrentModification(() {
It is intentional, but I can't find any places where we documented that
intention.
I've added it to the documentation of the List class that iteration really is
iteration of indices, and if the set of valid indices doesn't change between
iteration steps, there is no problem.

[Lasse Reichstein Nielsen, 2013-04-24 09:34:27]

Committed patchset #2 manually as r21935 (presubmit successful).

[hausner, 2013-04-25 23:07:17]

DBC

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

https://codereview.chromium.org/14468004/diff/7001/runtime/lib/growable_array...
runtime/lib/growable_array.dart:244: if (length != initialLength) throw new
ConcurrentModificationError(this);
This is not sufficient to detect modifications. The mutator could remove one
element and add one. That would go undetected with this check.

I think you need a counter in the array that gets incremented each time the list
is modified. Whether it is worth paying the price for detecting this is another
question.

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

https://codereview.chromium.org/14468004/diff/7001/sdk/lib/core/list.dart#new...
sdk/lib/core/list.dart:47: * the iterator will not detect it.
Right. So why add the check?

[Ivan Posva, 2013-04-26 05:58:22]

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

https://codereview.chromium.org/14468004/diff/7001/runtime/lib/growable_array...
runtime/lib/growable_array.dart:244: if (length != initialLength) throw new
ConcurrentModificationError(this);
On 2013/04/25 23:07:17, hausner wrote:
> This is not sufficient to detect modifications. The mutator could remove one
> element and add one. That would go undetected with this check.
> 
> I think you need a counter in the array that gets incremented each time the
list
> is modified. Whether it is worth paying the price for detecting this is
another
> question.
> 

I think the only concern is that length could change. Basically if you asked for
the length of this List, you'd expect that forEach is called the same number of
times.