Implement constructor kinds in the VM mirrors.

Implement constructor kinds in the VM mirrors.

BUG=http://dartbug.com/13798
R=asiva@google.com, hausner@google.com

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

[rmacnak, 2013-10-09 01:24:43]

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

https://codereview.chromium.org/26436004/diff/5001/runtime/vm/object.h#newcod...
runtime/vm/object.h:1866: kRedirectingBit = 11,
And now all 16 bits are used :/

[hausner, 2013-10-09 05:23:03]

LGTM with comments

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

https://codereview.chromium.org/26436004/diff/5001/runtime/vm/object.h#newcod...
runtime/vm/object.h:1866: kRedirectingBit = 11,
Dang, I should have grabbed this bit a long time ago to mark implicit
constructors :)

On 2013/10/09 01:24:43, Ryan Macnak wrote:
> And now all 16 bits are used :/

https://codereview.chromium.org/26436004/diff/5001/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/26436004/diff/5001/runtime/vm/parser.cc#newco...
runtime/vm/parser.cc:3156: // Redirected constructor: either this(...) or
this.xxx(...).
Why don't you set the redirecting flag here (or remember that it is redirecting
and set it below where the function object is created) instead of when the
constructor is actually compiled?

[siva, 2013-10-09 16:05:49]

https://codereview.chromium.org/26436004/diff/5001/tests/lib/mirrors/construc...
File tests/lib/mirrors/constructor_kinds_test.dart (right):

https://codereview.chromium.org/26436004/diff/5001/tests/lib/mirrors/construc...
tests/lib/mirrors/constructor_kinds_test.dart:97: mm =
cm.constructors[#Class.constRedirectingFactory];
I was initially confused by this naming pattern for constructors,
maybe it would be more readable to make the names explicit
Class.generativeConstructor
Class.redirectingGenerativeConstructor
Class.factoryConstructor
Class.redirectingFactoryConstructor
Class.constGenerativeConstructor
Class.constRedirectingGenerativeConstructor
Class.constRedirectingFactoryConstructor

[rmacnak, 2013-10-09 17:13:50]

https://codereview.chromium.org/26436004/diff/5001/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/26436004/diff/5001/runtime/vm/parser.cc#newco...
runtime/vm/parser.cc:3156: // Redirected constructor: either this(...) or
this.xxx(...).
On 2013/10/09 05:23:03, hausner wrote:
> Why don't you set the redirecting flag here (or remember that it is
redirecting
> and set it below where the function object is created) instead of when the
> constructor is actually compiled?

Yes, that is better. I didn't notice because I had left constructor calls in the
test to ensure the constructors could compile. I turned the test into a
multitest with and without the calls to cover this. Also on dart2js it is
interesting to test whether the constructor information is retained when there
are no base-level calls.

https://codereview.chromium.org/26436004/diff/5001/tests/lib/mirrors/construc...
File tests/lib/mirrors/constructor_kinds_test.dart (right):

https://codereview.chromium.org/26436004/diff/5001/tests/lib/mirrors/construc...
tests/lib/mirrors/constructor_kinds_test.dart:97: mm =
cm.constructors[#Class.constRedirectingFactory];
On 2013/10/09 16:05:50, siva wrote:
> I was initially confused by this naming pattern for constructors,
> maybe it would be more readable to make the names explicit
> Class.generativeConstructor
> Class.redirectingGenerativeConstructor
> Class.factoryConstructor
> Class.redirectingFactoryConstructor
> Class.constGenerativeConstructor
> Class.constRedirectingGenerativeConstructor
> Class.constRedirectingFactoryConstructor

Fixed. I notice the VM internally is inconsistent about whether constructor
means generative-or-factory-constructor (per spec) or means
generative-constructor. Presumably this reflects an earlier version of Dart
without factory constructors.

[siva, 2013-10-09 18:47:40]

lgtm

[rmacnak, 2013-10-09 20:40:18]

Committed patchset #5 manually as r28416 (presubmit successful).

[ahe, 2013-10-10 15:49:32]

Test LGTM!

https://codereview.chromium.org/26436004/diff/28001/tests/lib/mirrors/constru...
File tests/lib/mirrors/constructor_kinds_test.dart (right):

https://codereview.chromium.org/26436004/diff/28001/tests/lib/mirrors/constru...
tests/lib/mirrors/constructor_kinds_test.dart:34: // are retain even if there
are no base-level calls.
Great idea!