‘Yield* e' should not access e.current eagerly

‘Yield* e' should not access e.current eagerly

Match the implementation of dart2js.

BUG=
R=floitsch@google.com

Committed: https://github.com/dart-lang/sdk/commit/761ee7ec042670e11742e6e1b99c1973c1ce750c

[hausner, 2016-02-12 20:58:30]

Description was changed from

==========
‘Yield* e' should not access e.current eagerly

Match the implementation of dart2js.

BUG=
==========

to

==========
‘Yield* e' should not access e.current eagerly

Match the implementation of dart2js.

BUG=
==========

[hausner, 2016-02-12 20:58:30]

hausner@google.com changed reviewers:
+ regis@google.com

[floitsch, 2016-02-12 21:01:54]

floitsch@google.com changed reviewers:
+ floitsch@google.com

[floitsch, 2016-02-12 21:01:54]

LGTM.

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

https://codereview.chromium.org/1682173008/diff/1/runtime/lib/core_patch.dart...
runtime/lib/core_patch.dart:72: yieldEachIterator = (_current as
Iterable).iterator;
You could GC the _current here.
(_current = null)

[hausner, 2016-02-12 21:27:16]

Thank you.

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

https://codereview.chromium.org/1682173008/diff/1/runtime/lib/core_patch.dart...
runtime/lib/core_patch.dart:72: yieldEachIterator = (_current as
Iterable).iterator;
On 2016/02/12 21:01:54, floitsch wrote:
> You could GC the _current here.
> (_current = null)

Done.

[hausner, 2016-02-12 21:28:20]

Description was changed from

==========
‘Yield* e' should not access e.current eagerly

Match the implementation of dart2js.

BUG=
==========

to

==========
‘Yield* e' should not access e.current eagerly

Match the implementation of dart2js.

BUG=
R=floitsch@google.com

Committed:
https://github.com/dart-lang/sdk/commit/761ee7ec042670e11742e6e1b99c1973c1ce750c
==========

[hausner, 2016-02-12 21:28:21]

Committed patchset #2 (id:20001) manually as
761ee7ec042670e11742e6e1b99c1973c1ce750c (presubmit successful).

[Ivan Posva, 2016-02-13 07:03:03]

iposva@google.com changed reviewers:
+ iposva@google.com

[Ivan Posva, 2016-02-13 07:03:03]

https://codereview.chromium.org/1682173008/diff/20001/runtime/lib/core_patch....
File runtime/lib/core_patch.dart (right):

https://codereview.chromium.org/1682173008/diff/20001/runtime/lib/core_patch....
runtime/lib/core_patch.dart:69: if (isYieldEach) {
Can you please explain in a comment how this code here is ever reached? Unless
of course the moveNextFn updates the isYieldEach value of this iterator when it
is being called, but such a side-effect should definitely be pointed out.

Thanks!

[hausner, 2016-02-13 07:13:06]

https://codereview.chromium.org/1682173008/diff/20001/runtime/lib/core_patch....
File runtime/lib/core_patch.dart (right):

https://codereview.chromium.org/1682173008/diff/20001/runtime/lib/core_patch....
runtime/lib/core_patch.dart:69: if (isYieldEach) {
On 2016/02/13 07:03:03, Ivan Posva wrote:
> Can you please explain in a comment how this code here is ever reached? Unless
> of course the moveNextFn updates the isYieldEach value of this iterator when
it
> is being called, but such a side-effect should definitely be pointed out.
> 
> Thanks!

Yes, the generator sets this value when it reaches a yield* statement. See
comment in line 41.

[Ivan Posva, 2016-02-13 07:27:09]

Thanks for the clarification. Should I add the suggested comment?

-Ivan

https://codereview.chromium.org/1682173008/diff/20001/runtime/lib/core_patch....
File runtime/lib/core_patch.dart (right):

https://codereview.chromium.org/1682173008/diff/20001/runtime/lib/core_patch....
runtime/lib/core_patch.dart:69: if (isYieldEach) {
On 2016/02/13 07:13:06, hausner wrote:
> On 2016/02/13 07:03:03, Ivan Posva wrote:
> > Can you please explain in a comment how this code here is ever reached?
Unless
> > of course the moveNextFn updates the isYieldEach value of this iterator when
> it
> > is being called, but such a side-effect should definitely be pointed out.
> > 
> > Thanks!
> 
> Yes, the generator sets this value when it reaches a yield* statement. See
> comment in line 41.

I understood the above comment as the field being set once on construction,
which apparently is not the case.

How about a comment like this before line 64:
// The moveNextFn will update the values of the isYieldEach and _current fields.

Note on the side: Why are we publicly exposing the internal fields of this
implementation class?