Skeleton typedef type implementation

Skeleton typedef type implementation

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

[ahe, 2012-08-03 15:48:12]

lgtm

[Johnni Winther, 2012-08-14 13:25:31]

PTAL

[ngeoffray, 2012-08-16 12:45:44]

DBC

https://chromiumcodereview.appspot.com/10832136/diff/7002/lib/compiler/implem...
File lib/compiler/implementation/resolver.dart (right):

https://chromiumcodereview.appspot.com/10832136/diff/7002/lib/compiler/implem...
lib/compiler/implementation/resolver.dart:1814: visitClassNode(ClassNode node) {
Why this change?

https://chromiumcodereview.appspot.com/10832136/diff/7002/lib/compiler/implem...
File lib/compiler/implementation/typechecker.dart (right):

https://chromiumcodereview.appspot.com/10832136/diff/7002/lib/compiler/implem...
lib/compiler/implementation/typechecker.dart:41: Type unalias(Compiler
compiler);
Why having an 'unalias' function and not a isFunction method? I think you're
commenting at each call site why you're calling 'unalias', and a predicate
method would actually be clearer.

https://chromiumcodereview.appspot.com/10832136/diff/7002/lib/compiler/implem...
lib/compiler/implementation/typechecker.dart:159: // TODO(ahe): This should
[ensureResolved].
should *call*

[ahe, 2012-08-16 13:22:58]

LGTM, you can do the ensureResolved change in the next CL.

https://chromiumcodereview.appspot.com/10832136/diff/7002/lib/compiler/implem...
File lib/compiler/implementation/elements/elements.dart (right):

https://chromiumcodereview.appspot.com/10832136/diff/7002/lib/compiler/implem...
lib/compiler/implementation/elements/elements.dart:482: bool isResolved = false;
Can you use states instead as I do in ClassElement?

https://chromiumcodereview.appspot.com/10832136/diff/7002/lib/compiler/implem...
lib/compiler/implementation/elements/elements.dart:494: * resolved.
How do you resolve a typedef element?

https://chromiumcodereview.appspot.com/10832136/diff/7002/lib/compiler/implem...
File lib/compiler/implementation/native_handler.dart (right):

https://chromiumcodereview.appspot.com/10832136/diff/7002/lib/compiler/implem...
lib/compiler/implementation/native_handler.dart:253: input =
convertDartClosure(parameter, type);
I suspect that we want to pass the unaliased type here eventually.

https://chromiumcodereview.appspot.com/10832136/diff/7002/lib/compiler/implem...
File lib/compiler/implementation/resolver.dart (right):

https://chromiumcodereview.appspot.com/10832136/diff/7002/lib/compiler/implem...
lib/compiler/implementation/resolver.dart:379: void
resolveTypedef(TypedefElement element) {
Could you make this look more like resolveClass and ClassElement.ensureResolved?

https://chromiumcodereview.appspot.com/10832136/diff/7002/lib/compiler/implem...
lib/compiler/implementation/resolver.dart:382: return
compiler.withCurrentElement(element, () {
You can use:
..., () => measure() {

This is shorter and reduces the number of braces.

https://chromiumcodereview.appspot.com/10832136/diff/7002/lib/compiler/implem...
lib/compiler/implementation/resolver.dart:824: // TODO(ahe): Should be
[ensureResolved].
I think you have to do this now :-)

https://chromiumcodereview.appspot.com/10832136/diff/7002/lib/compiler/implem...
File lib/compiler/implementation/typechecker.dart (right):

https://chromiumcodereview.appspot.com/10832136/diff/7002/lib/compiler/implem...
lib/compiler/implementation/typechecker.dart:41: Type unalias(Compiler
compiler);
On 2012/08/16 12:45:44, ngeoffray wrote:
> Why having an 'unalias' function and not a isFunction method? I think you're
> commenting at each call site why you're calling 'unalias', and a predicate
> method would actually be clearer.

I was thinking the same.

Clearly, unalias is the general operation we need, but a convenient predicate
would make the code easier to read.

https://chromiumcodereview.appspot.com/10832136/diff/7002/lib/compiler/implem...
lib/compiler/implementation/typechecker.dart:185: // TODO(karlklose): should we
have a class Void?
This TODO really belongs on line 178.

[Johnni Winther, 2012-08-17 11:15:19]

https://chromiumcodereview.appspot.com/10832136/diff/7002/lib/compiler/implem...
File lib/compiler/implementation/elements/elements.dart (right):

https://chromiumcodereview.appspot.com/10832136/diff/7002/lib/compiler/implem...
lib/compiler/implementation/elements/elements.dart:482: bool isResolved = false;
On 2012/08/16 13:22:58, ahe wrote:
> Can you use states instead as I do in ClassElement?

I will do that in a later CL.

https://chromiumcodereview.appspot.com/10832136/diff/7002/lib/compiler/implem...
lib/compiler/implementation/elements/elements.dart:494: * resolved.
On 2012/08/16 13:22:58, ahe wrote:
> How do you resolve a typedef element?

Currently `compiler.resolveTypedef(this)`. I will change it use the
[ensureResolved] scheme in a later CL.

https://chromiumcodereview.appspot.com/10832136/diff/7002/lib/compiler/implem...
File lib/compiler/implementation/native_handler.dart (right):

https://chromiumcodereview.appspot.com/10832136/diff/7002/lib/compiler/implem...
lib/compiler/implementation/native_handler.dart:253: input =
convertDartClosure(parameter, type);
On 2012/08/16 13:22:58, ahe wrote:
> I suspect that we want to pass the unaliased type here eventually.

It seems that what is need is the arity, which implies that type is must be a
FunctionType.

https://chromiumcodereview.appspot.com/10832136/diff/7002/lib/compiler/implem...
File lib/compiler/implementation/resolver.dart (right):

https://chromiumcodereview.appspot.com/10832136/diff/7002/lib/compiler/implem...
lib/compiler/implementation/resolver.dart:379: void
resolveTypedef(TypedefElement element) {
On 2012/08/16 13:22:58, ahe wrote:
> Could you make this look more like resolveClass and
ClassElement.ensureResolved?

I will do that in a later CL.

https://chromiumcodereview.appspot.com/10832136/diff/7002/lib/compiler/implem...
lib/compiler/implementation/resolver.dart:382: return
compiler.withCurrentElement(element, () {
On 2012/08/16 13:22:58, ahe wrote:
> You can use:
> ..., () => measure() {
> 
> This is shorter and reduces the number of braces.

I will do that in a later CL.

https://chromiumcodereview.appspot.com/10832136/diff/7002/lib/compiler/implem...
lib/compiler/implementation/resolver.dart:824: // TODO(ahe): Should be
[ensureResolved].
On 2012/08/16 13:22:58, ahe wrote:
> I think you have to do this now :-)

I will do that in a later CL.

https://chromiumcodereview.appspot.com/10832136/diff/7002/lib/compiler/implem...
lib/compiler/implementation/resolver.dart:1814: visitClassNode(ClassNode node) {
On 2012/08/16 12:45:44, ngeoffray wrote:
> Why this change?

A leftover from another change which hasn't been committed yet.

https://chromiumcodereview.appspot.com/10832136/diff/7002/lib/compiler/implem...
File lib/compiler/implementation/typechecker.dart (right):

https://chromiumcodereview.appspot.com/10832136/diff/7002/lib/compiler/implem...
lib/compiler/implementation/typechecker.dart:41: Type unalias(Compiler
compiler);
On 2012/08/16 13:22:58, ahe wrote:
> On 2012/08/16 12:45:44, ngeoffray wrote:
> > Why having an 'unalias' function and not a isFunction method? I think you're
> > commenting at each call site why you're calling 'unalias', and a predicate
> > method would actually be clearer.
> 
> I was thinking the same.
> 
> Clearly, unalias is the general operation we need, but a convenient predicate
> would make the code easier to read.

The convenience method would have to have the signature `bool isFunct(Compiler
compiler)` in order to compute the alias, and it doesn't seem so convenient.

https://chromiumcodereview.appspot.com/10832136/diff/7002/lib/compiler/implem...
lib/compiler/implementation/typechecker.dart:159: // TODO(ahe): This should
[ensureResolved].
On 2012/08/16 12:45:44, ngeoffray wrote:
> should *call*

Done.

https://chromiumcodereview.appspot.com/10832136/diff/7002/lib/compiler/implem...
lib/compiler/implementation/typechecker.dart:185: // TODO(karlklose): should we
have a class Void?
On 2012/08/16 13:22:58, ahe wrote:
> This TODO really belongs on line 178.

Done.

[ahe, 2012-08-21 14:27:24]

FYI

https://chromiumcodereview.appspot.com/10832136/diff/7002/lib/compiler/implem...
File lib/compiler/implementation/elements/elements.dart (right):

https://chromiumcodereview.appspot.com/10832136/diff/7002/lib/compiler/implem...
lib/compiler/implementation/elements/elements.dart:494: * resolved.
On 2012/08/17 11:15:19, Johnni Winther wrote:
> On 2012/08/16 13:22:58, ahe wrote:
> > How do you resolve a typedef element?
> 
> Currently `compiler.resolveTypedef(this)`. I will change it use the
> [ensureResolved] scheme in a later CL.

I hoped you would include this explanation in the comment, not a direct reply.
I'll try to be more obvious about that.

[ahe, 2012-08-21 14:28:38]

https://chromiumcodereview.appspot.com/10832136/diff/7002/lib/compiler/implem...
File lib/compiler/implementation/typechecker.dart (right):

https://chromiumcodereview.appspot.com/10832136/diff/7002/lib/compiler/implem...
lib/compiler/implementation/typechecker.dart:41: Type unalias(Compiler
compiler);
On 2012/08/17 11:15:19, Johnni Winther wrote:
> On 2012/08/16 13:22:58, ahe wrote:
> > On 2012/08/16 12:45:44, ngeoffray wrote:
> > > Why having an 'unalias' function and not a isFunction method? I think
you're
> > > commenting at each call site why you're calling 'unalias', and a predicate
> > > method would actually be clearer.
> > 
> > I was thinking the same.
> > 
> > Clearly, unalias is the general operation we need, but a convenient
predicate
> > would make the code easier to read.
> 
> The convenience method would have to have the signature `bool isFunct(Compiler
> compiler)` in order to compute the alias, and it doesn't seem so convenient.

You can answer the question "is function?" without computing the alias.

[Johnni Winther, 2012-08-22 07:47:58]

https://chromiumcodereview.appspot.com/10832136/diff/7002/lib/compiler/implem...
File lib/compiler/implementation/typechecker.dart (right):

https://chromiumcodereview.appspot.com/10832136/diff/7002/lib/compiler/implem...
lib/compiler/implementation/typechecker.dart:41: Type unalias(Compiler
compiler);
On 2012/08/21 14:28:39, ahe wrote:
> On 2012/08/17 11:15:19, Johnni Winther wrote:
> > On 2012/08/16 13:22:58, ahe wrote:
> > > On 2012/08/16 12:45:44, ngeoffray wrote:
> > > > Why having an 'unalias' function and not a isFunction method? I think
> you're
> > > > commenting at each call site why you're calling 'unalias', and a
predicate
> > > > method would actually be clearer.
> > > 
> > > I was thinking the same.
> > > 
> > > Clearly, unalias is the general operation we need, but a convenient
> predicate
> > > would make the code easier to read.
> > 
> > The convenience method would have to have the signature `bool
isFunct(Compiler
> > compiler)` in order to compute the alias, and it doesn't seem so convenient.
> 
> You can answer the question "is function?" without computing the alias.

No. That is why we need Compiler.

[ahe, 2012-08-22 07:50:19]

https://chromiumcodereview.appspot.com/10832136/diff/7002/lib/compiler/implem...
File lib/compiler/implementation/typechecker.dart (right):

https://chromiumcodereview.appspot.com/10832136/diff/7002/lib/compiler/implem...
lib/compiler/implementation/typechecker.dart:41: Type unalias(Compiler
compiler);
On 2012/08/22 07:47:58, Johnni Winther wrote:
> On 2012/08/21 14:28:39, ahe wrote:
> > On 2012/08/17 11:15:19, Johnni Winther wrote:
> > > On 2012/08/16 13:22:58, ahe wrote:
> > > > On 2012/08/16 12:45:44, ngeoffray wrote:
> > > > > Why having an 'unalias' function and not a isFunction method? I think
> > you're
> > > > > commenting at each call site why you're calling 'unalias', and a
> predicate
> > > > > method would actually be clearer.
> > > > 
> > > > I was thinking the same.
> > > > 
> > > > Clearly, unalias is the general operation we need, but a convenient
> > predicate
> > > > would make the code easier to read.
> > > 
> > > The convenience method would have to have the signature `bool
> isFunct(Compiler
> > > compiler)` in order to compute the alias, and it doesn't seem so
convenient.
> > 
> > You can answer the question "is function?" without computing the alias.
> 
> No. That is why we need Compiler.

You don't need the compiler because the only TypedefElement you have is a
function type.