Futures.wait should call then, even if some(or all) of the futures are done

Futures.wait should call then, even if some(or all) of the futures are done
Committed: https://code.google.com/p/dart/source/detail?r=3033

[kasperl, 2012-01-05 17:09:49]

Adding Siggi as a reviewer (hope that's okay).

[kasperl, 2012-01-06 08:17:53]

Thanks a lot for doing this, Anders. LGTM if you add a test case that would fail
before your fix.

http://codereview.chromium.org/9108010/diff/3001/corelib/src/future.dart
File corelib/src/future.dart (right):

http://codereview.chromium.org/9108010/diff/3001/corelib/src/future.dart#newc...
corelib/src/future.dart:137: int pos = i;
Maybe move the declaration of pos (and the TODO) down to the point where you
need it (before the invocation of then where you capture it) and use i in the
place where that works fine?

http://codereview.chromium.org/9108010/diff/3001/corelib/src/future.dart#newc...
corelib/src/future.dart:139: remaining--;
Nit: Maybe update remaining after storing the value in values to be consistent
with the postponed 'then' case.

[Anders Johnsen, 2012-01-06 08:57:09]

I've attached a test file. Let me know if you find it sufficient.

Thank you!

http://codereview.chromium.org/9108010/diff/3001/corelib/src/future.dart
File corelib/src/future.dart (right):

http://codereview.chromium.org/9108010/diff/3001/corelib/src/future.dart#newc...
corelib/src/future.dart:137: int pos = i;
On 2012/01/06 08:17:53, kasperl wrote:
> Maybe move the declaration of pos (and the TODO) down to the point where you
> need it (before the invocation of then where you capture it) and use i in the
> place where that works fine?

Done.

http://codereview.chromium.org/9108010/diff/3001/corelib/src/future.dart#newc...
corelib/src/future.dart:139: remaining--;
On 2012/01/06 08:17:53, kasperl wrote:
> Nit: Maybe update remaining after storing the value in values to be consistent
> with the postponed 'then' case.

Done.

[kasperl, 2012-01-06 09:06:46]

http://codereview.chromium.org/9108010/diff/5003/tests/corelib/src/FuturesTes...
File tests/corelib/src/FuturesTest.dart (right):

http://codereview.chromium.org/9108010/diff/5003/tests/corelib/src/FuturesTes...
tests/corelib/src/FuturesTest.dart:17: Expect.equals(list.length, 3);
Weird indentation. Emacs user?

http://codereview.chromium.org/9108010/diff/5003/tests/corelib/src/FuturesTes...
tests/corelib/src/FuturesTest.dart:17: Expect.equals(list.length, 3);
Expect.equals(3, list.length) -- expectation, actual

http://codereview.chromium.org/9108010/diff/5003/tests/corelib/src/FuturesTes...
tests/corelib/src/FuturesTest.dart:22: static void testWaitEmpty() {
Your void methods return values. Seems wrong.

http://codereview.chromium.org/9108010/diff/5003/tests/corelib/src/FuturesTes...
tests/corelib/src/FuturesTest.dart:32: 
I would use less newlines here.

http://codereview.chromium.org/9108010/diff/5003/tests/corelib/src/FuturesTes...
tests/corelib/src/FuturesTest.dart:44: 
.. and here.

http://codereview.chromium.org/9108010/diff/5003/tests/corelib/src/FuturesTes...
tests/corelib/src/FuturesTest.dart:52: FuturesTest.testMain();
Maybe this would actually be nicer without the class. Why not just use top-level
functions instead?

[Anders Johnsen, 2012-01-06 09:28:32]

Let me know if you still want me to remove the class from the test file. If not,
I'll commit :)

http://codereview.chromium.org/9108010/diff/5003/tests/corelib/src/FuturesTes...
File tests/corelib/src/FuturesTest.dart (right):

http://codereview.chromium.org/9108010/diff/5003/tests/corelib/src/FuturesTes...
tests/corelib/src/FuturesTest.dart:17: Expect.equals(list.length, 3);
On 2012/01/06 09:06:46, kasperl wrote:
> Weird indentation. Emacs user?

Vim, fixed :)

http://codereview.chromium.org/9108010/diff/5003/tests/corelib/src/FuturesTes...
tests/corelib/src/FuturesTest.dart:22: static void testWaitEmpty() {
On 2012/01/06 09:06:46, kasperl wrote:
> Your void methods return values. Seems wrong.

Done.

http://codereview.chromium.org/9108010/diff/5003/tests/corelib/src/FuturesTes...
tests/corelib/src/FuturesTest.dart:32: 
On 2012/01/06 09:06:46, kasperl wrote:
> I would use less newlines here.

Done.

http://codereview.chromium.org/9108010/diff/5003/tests/corelib/src/FuturesTes...
tests/corelib/src/FuturesTest.dart:44: 
On 2012/01/06 09:06:46, kasperl wrote:
> .. and here.

Done.

http://codereview.chromium.org/9108010/diff/5003/tests/corelib/src/FuturesTes...
tests/corelib/src/FuturesTest.dart:52: FuturesTest.testMain();
On 2012/01/06 09:06:46, kasperl wrote:
> Maybe this would actually be nicer without the class. Why not just use
top-level
> functions instead?

Yeah, but using a class with static members follows the style of the other
tests. Want me to remove the class?

[Anders Johnsen, 2012-01-06 12:42:17]

Removed surrounding class and committed, thank you!

[Siggi Cherem (dart-lang), 2012-01-10 18:56:43]

http://codereview.chromium.org/9108010/diff/11002/corelib/src/future.dart
File corelib/src/future.dart (right):

http://codereview.chromium.org/9108010/diff/11002/corelib/src/future.dart#new...
corelib/src/future.dart:135: if (futures[i].isComplete) {
(sorry for the slow reply I was OOO until today)

We might be able to simplify this change slightly.

We need overall more tests for this code (thanks for the new tests!), but I
believe many of the tests you added, except [testWaitEmpty] worked before your
change too. I believe all other test were passing before because [Future.then]
internally calls the complete handler immediately if the future is already
completed (basically the same code you have inlined here in this branch).

I believe we can simplify this by removing this conditional (reverting the body
of the loop as before), but changing line 152 below to say 'futures.length == 0'
rather than 'remaining == 0'.

[Anders Johnsen, 2012-01-10 19:21:39]

http://codereview.chromium.org/9108010/diff/11002/corelib/src/future.dart
File corelib/src/future.dart (right):

http://codereview.chromium.org/9108010/diff/11002/corelib/src/future.dart#new...
corelib/src/future.dart:135: if (futures[i].isComplete) {
On 2012/01/10 18:56:43, sigmund wrote:
> (sorry for the slow reply I was OOO until today)
> 
> We might be able to simplify this change slightly.
> 
> We need overall more tests for this code (thanks for the new tests!), but I
> believe many of the tests you added, except [testWaitEmpty] worked before your
> change too. I believe all other test were passing before because [Future.then]
> internally calls the complete handler immediately if the future is already
> completed (basically the same code you have inlined here in this branch).
> 
> I believe we can simplify this by removing this conditional (reverting the
body
> of the loop as before), but changing line 152 below to say 'futures.length ==
0'
> rather than 'remaining == 0'.

I was having problems with futures being complete not responding, so I'll
investigate that a bit further.

And yes, that is a nicer way to write it.

Thank you for the review!