Add support for sync* and yield

Add support for sync* and yield and yield*

R=srdjan@google.com

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

[hausner, 2015-01-29 00:13:02]

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

[hausner, 2015-01-29 00:13:30]

Will add a couple of tests while you take a look at this.

[hausner, 2015-01-29 00:13:30]

Will add a couple of tests while you take a look at this.

[hausner, 2015-01-30 22:00:21]

Added tests. Implemented spec-compliant rules when async, await and yield are
reserved words with special meaning and when they are not.

[hausner, 2015-02-03 20:56:50]

Updated test expectations and fixed new async_await_syntax_test.

[hausner, 2015-02-04 00:46:38]

Now also with support for yield*.

[srdjan, 2015-02-04 19:29:11]

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

[srdjan, 2015-02-04 19:29:12]

First round of comments.

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

https://codereview.chromium.org/888463004/diff/140001/runtime/lib/core_patch....
runtime/lib/core_patch.dart:37: var isYieldEach;
Please add comment who sets isYieldEach. Also maybe add some comments to both
_SyncIterable and _SyncIterator.

https://codereview.chromium.org/888463004/diff/140001/runtime/lib/core_patch....
runtime/lib/core_patch.dart:39: var current = null;
I think null is the default initialization for all fields, you do not need to
explicitly do it.

https://codereview.chromium.org/888463004/diff/140001/runtime/lib/core_patch....
runtime/lib/core_patch.dart:40: var moveNextFn;
Does the spec say anything about the type of moveNextFn? If so, please use type.

https://codereview.chromium.org/888463004/diff/140001/runtime/lib/core_patch....
runtime/lib/core_patch.dart:44: moveNext() {
bool moveNext() {

https://codereview.chromium.org/888463004/diff/140001/runtime/lib/core_patch....
runtime/lib/core_patch.dart:48: for(;;) {
What is the advantage of "for(;;)" vs "while(true)"? (I find the later more
readable).

https://codereview.chromium.org/888463004/diff/140001/runtime/lib/function.cc
File runtime/lib/function.cc (right):

https://codereview.chromium.org/888463004/diff/140001/runtime/lib/function.cc...
runtime/lib/function.cc:97: return Object::null();
Should that be UNREACHABLE()?

https://codereview.chromium.org/888463004/diff/140001/runtime/vm/flow_graph_b...
File runtime/vm/flow_graph_builder.cc (right):

https://codereview.chromium.org/888463004/diff/140001/runtime/vm/flow_graph_b...
runtime/vm/flow_graph_builder.cc:3798: const Function& function =
owner()->parsed_function().function();
owner()->function();

https://codereview.chromium.org/888463004/diff/140001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/888463004/diff/140001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:12791: const char* format = "%s\nvar %s  token-pos %ld  ctx
lvl %ld  index %ld";
I am not sure %ld will fly on all platforms. Maybe use %" Pd " instead.

https://codereview.chromium.org/888463004/diff/140001/runtime/vm/object.h
File runtime/vm/object.h (right):

https://codereview.chromium.org/888463004/diff/140001/runtime/vm/object.h#new...
runtime/vm/object.h:2155: bool IsAsyncClosure() const {
ASSERT(IsLocalFUnction) or that parent_function() is not NULL.

https://codereview.chromium.org/888463004/diff/140001/runtime/vm/object.h#new...
runtime/vm/object.h:2168: bool IsSyncGenClosure() const {
ditto

[hausner, 2015-02-04 22:09:24]

Merged in async/await stack trace changes and addressed first set of comments.

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

https://codereview.chromium.org/888463004/diff/140001/runtime/lib/core_patch....
runtime/lib/core_patch.dart:37: var isYieldEach;
On 2015/02/04 19:29:12, srdjan wrote:
> Please add comment who sets isYieldEach. Also maybe add some comments to both
> _SyncIterable and _SyncIterator.

Done.

https://codereview.chromium.org/888463004/diff/140001/runtime/lib/core_patch....
runtime/lib/core_patch.dart:39: var current = null;
On 2015/02/04 19:29:12, srdjan wrote:
> I think null is the default initialization for all fields, you do not need to
> explicitly do it.

Done.

https://codereview.chromium.org/888463004/diff/140001/runtime/lib/core_patch....
runtime/lib/core_patch.dart:40: var moveNextFn;
On 2015/02/04 19:29:12, srdjan wrote:
> Does the spec say anything about the type of moveNextFn? If so, please use
type.

I don't want to use types. The type checks are unnecessary runtime overhead.

https://codereview.chromium.org/888463004/diff/140001/runtime/lib/core_patch....
runtime/lib/core_patch.dart:44: moveNext() {
On 2015/02/04 19:29:12, srdjan wrote:
> bool moveNext() {

Done.

https://codereview.chromium.org/888463004/diff/140001/runtime/lib/core_patch....
runtime/lib/core_patch.dart:48: for(;;) {
On 2015/02/04 19:29:12, srdjan wrote:
> What is the advantage of "for(;;)" vs "while(true)"? (I find the later more
> readable).

Done.

https://codereview.chromium.org/888463004/diff/140001/runtime/lib/function.cc
File runtime/lib/function.cc (right):

https://codereview.chromium.org/888463004/diff/140001/runtime/lib/function.cc...
runtime/lib/function.cc:97: return Object::null();
On 2015/02/04 19:29:12, srdjan wrote:
> Should that be UNREACHABLE()?

I modeled this after FunctionImpl_equals above. There is an assertion that the
parameter is a closure, so we do have a check in debug mode. I don't have a
strong preference.

https://codereview.chromium.org/888463004/diff/140001/runtime/vm/flow_graph_b...
File runtime/vm/flow_graph_builder.cc (right):

https://codereview.chromium.org/888463004/diff/140001/runtime/vm/flow_graph_b...
runtime/vm/flow_graph_builder.cc:3798: const Function& function =
owner()->parsed_function().function();
On 2015/02/04 19:29:12, srdjan wrote:
> owner()->function();

Done.

https://codereview.chromium.org/888463004/diff/140001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/888463004/diff/140001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:12791: const char* format = "%s\nvar %s  token-pos %ld  ctx
lvl %ld  index %ld";
On 2015/02/04 19:29:12, srdjan wrote:
> I am not sure %ld will fly on all platforms. Maybe use %" Pd " instead.

Right. Done.

https://codereview.chromium.org/888463004/diff/140001/runtime/vm/object.h
File runtime/vm/object.h (right):

https://codereview.chromium.org/888463004/diff/140001/runtime/vm/object.h#new...
runtime/vm/object.h:2155: bool IsAsyncClosure() const {
On 2015/02/04 19:29:12, srdjan wrote:
> ASSERT(IsLocalFUnction) or that parent_function() is not NULL.

is_generated_function() implies that it's a local function. I can't do the
assert here because the caller doesn't have to guarantee that IsAsyncClosure
only gets called for local functions.

I would have to write this as 

if (is_generated_closure()) {
  ASSERT(IsLocalFunction());
  return Function::Handle(parent_function()).IsAsyncFunction();
}
return false;

which I think is a bit over the top.

https://codereview.chromium.org/888463004/diff/140001/runtime/vm/object.h#new...
runtime/vm/object.h:2168: bool IsSyncGenClosure() const {
On 2015/02/04 19:29:12, srdjan wrote:
> ditto

ditto.

[hausner, 2015-02-05 00:41:07]

Added type info to the core_lib code. Also fixed implicit return statement to
return false for sync generator code.

[srdjan, 2015-02-05 18:01:23]

LGTM with comments

https://codereview.chromium.org/888463004/diff/180001/tests/language/sync_gen...
File tests/language/sync_generator1_test.dart (right):

https://codereview.chromium.org/888463004/diff/180001/tests/language/sync_gen...
tests/language/sync_generator1_test.dart:43: main() {
Maybe you want to wrap all this into a loop/method, use low
optimization_counter_threshold so that it gets tested by the optimizing compiler
as well

https://codereview.chromium.org/888463004/diff/180001/tests/language/sync_gen...
File tests/language/sync_generator2_test.dart (right):

https://codereview.chromium.org/888463004/diff/180001/tests/language/sync_gen...
tests/language/sync_generator2_test.dart:41: set test04(a) sync* { print(a); } 
/// 41: compile-time error
Can you test it also for async, async* ? (It is a compile-time error if an
async, async* or sync* modifier is attached to the body of a setter or
constructor.)

[hausner, 2015-02-05 18:33:08]

New patchsets have been uploaded after l-g-t-m from srdjan@google.com

[hausner, 2015-02-05 18:34:51]

Thank you.

https://codereview.chromium.org/888463004/diff/180001/tests/language/sync_gen...
File tests/language/sync_generator1_test.dart (right):

https://codereview.chromium.org/888463004/diff/180001/tests/language/sync_gen...
tests/language/sync_generator1_test.dart:43: main() {
On 2015/02/05 18:01:22, srdjan wrote:
> Maybe you want to wrap all this into a loop/method, use low
> optimization_counter_threshold so that it gets tested by the optimizing
compiler
> as well

Done.

https://codereview.chromium.org/888463004/diff/180001/tests/language/sync_gen...
File tests/language/sync_generator2_test.dart (right):

https://codereview.chromium.org/888463004/diff/180001/tests/language/sync_gen...
tests/language/sync_generator2_test.dart:41: set test04(a) sync* { print(a); } 
/// 41: compile-time error
On 2015/02/05 18:01:22, srdjan wrote:
> Can you test it also for async, async* ? (It is a compile-time error if an
> async, async* or sync* modifier is attached to the body of a setter or
> constructor.)

As discussed offline, code with async and async* annotation is covered in other
tests. I wanted this test to only deal with async* functions.

[hausner, 2015-02-05 18:37:11]

Committed patchset #11 (id:200001) manually as r43516 (presubmit successful).