Implement named constructors and resolving of redirecting constructors and super-initializers.

Implement named constructors and resolving of redirecting constructors and super-initializers.

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

[floitsch, 2012-01-18 15:22:14]

LGTM!

http://codereview.chromium.org/9243011/diff/2001/frog/leg/resolver.dart
File frog/leg/resolver.dart (right):

http://codereview.chromium.org/9243011/diff/2001/frog/leg/resolver.dart#newco...
frog/leg/resolver.dart:120: if (initializerOrSuper == null) initializerOrSuper =
init;
initializerOrSuper is unconditionally reassigned below.

http://codereview.chromium.org/9243011/diff/2001/frog/leg/resolver.dart#newco...
frog/leg/resolver.dart:158: ClassElement lookupTarget =
constructor.enclosingElement;
new line after nested function.

http://codereview.chromium.org/9243011/diff/2001/frog/leg/resolver.dart#newco...
frog/leg/resolver.dart:189: target = lookupTarget.getSynthesizedConstructor();
what if there is no synthesized constructor?
The next line should probably be "if" instead of "else if" ?

http://codereview.chromium.org/9243011/diff/2001/frog/leg/resolver.dart#newco...
frog/leg/resolver.dart:196: final int parameters = targetNode.parameterCount();
no need to create these intermediate variables. parameterCount() and
argumentCount() are descriptive (even more than 'parameters' and 'arguments',
and if I'm not wrong the condition fits onto one line.

http://codereview.chromium.org/9243011/diff/2001/frog/leg/resolver.dart#newco...
frog/leg/resolver.dart:318: if (node.typeName.asSend() !== null) {
add comment when this happen (for 'new' and 'const'), and maybe for library
prefixes?

http://codereview.chromium.org/9243011/diff/2001/frog/leg/resolver.dart#newco...
frog/leg/resolver.dart:443: Identifier constructor =
node.asSend().selector.asIdentifier();
something missing here?

http://codereview.chromium.org/9243011/diff/2001/frog/leg/resolver.dart#newco...
frog/leg/resolver.dart:650: int parameterCount = fun.parameterCount();
ditto. no need to have these intermediate variables. (even though the names are
better here)

http://codereview.chromium.org/9243011/diff/2001/frog/leg/tree/nodes.dart
File frog/leg/tree/nodes.dart (right):

http://codereview.chromium.org/9243011/diff/2001/frog/leg/tree/nodes.dart#new...
frog/leg/tree/nodes.dart:250: receiver.asIdentifier().isSuper();
add selector.asIdentifier() !== null ?

http://codereview.chromium.org/9243011/diff/2001/frog/leg/warnings.dart
File frog/leg/warnings.dart (right):

http://codereview.chromium.org/9243011/diff/2001/frog/leg/warnings.dart#newco...
frog/leg/warnings.dart:64: 'only call to \'this\' or \'super\' constructor
allowed');
you could avoid the escapes by using " to delimit the string.

http://codereview.chromium.org/9243011/diff/2001/frog/leg/warnings.dart#newco...
frog/leg/warnings.dart:70: "'Object' cannot have a superinitializer");
super initializer

[karlklose, 2012-01-18 16:36:08]

Thanks Florian!

http://codereview.chromium.org/9243011/diff/2001/frog/leg/resolver.dart
File frog/leg/resolver.dart (right):

http://codereview.chromium.org/9243011/diff/2001/frog/leg/resolver.dart#newco...
frog/leg/resolver.dart:120: if (initializerOrSuper == null) initializerOrSuper =
init;
Done, moved.

http://codereview.chromium.org/9243011/diff/2001/frog/leg/resolver.dart#newco...
frog/leg/resolver.dart:158: ClassElement lookupTarget =
constructor.enclosingElement;
On 2012/01/18 15:22:14, floitsch wrote:
> new line after nested function.

Done.

http://codereview.chromium.org/9243011/diff/2001/frog/leg/resolver.dart#newco...
frog/leg/resolver.dart:189: target = lookupTarget.getSynthesizedConstructor();
On 2012/01/18 15:22:14, floitsch wrote:
> what if there is no synthesized constructor?
> The next line should probably be "if" instead of "else if" ?

Done.

http://codereview.chromium.org/9243011/diff/2001/frog/leg/resolver.dart#newco...
frog/leg/resolver.dart:196: final int parameters = targetNode.parameterCount();
On 2012/01/18 15:22:14, floitsch wrote:
> no need to create these intermediate variables. parameterCount() and
> argumentCount() are descriptive (even more than 'parameters' and 'arguments',
> and if I'm not wrong the condition fits onto one line.

Done.

http://codereview.chromium.org/9243011/diff/2001/frog/leg/resolver.dart#newco...
frog/leg/resolver.dart:318: if (node.typeName.asSend() !== null) {
On 2012/01/18 15:22:14, floitsch wrote:
> add comment when this happen (for 'new' and 'const'), and maybe for library
> prefixes?

Done.

http://codereview.chromium.org/9243011/diff/2001/frog/leg/resolver.dart#newco...
frog/leg/resolver.dart:443: Identifier constructor =
node.asSend().selector.asIdentifier();
On 2012/01/18 15:22:14, floitsch wrote:
> something missing here?

Done.

http://codereview.chromium.org/9243011/diff/2001/frog/leg/resolver.dart#newco...
frog/leg/resolver.dart:650: int parameterCount = fun.parameterCount();
On 2012/01/18 15:22:14, floitsch wrote:
> ditto. no need to have these intermediate variables. (even though the names
are
> better here)

Done.

http://codereview.chromium.org/9243011/diff/2001/frog/leg/tree/nodes.dart
File frog/leg/tree/nodes.dart (right):

http://codereview.chromium.org/9243011/diff/2001/frog/leg/tree/nodes.dart#new...
frog/leg/tree/nodes.dart:250: receiver.asIdentifier().isSuper();
On 2012/01/18 15:22:14, floitsch wrote:
> add selector.asIdentifier() !== null ?

Done.

http://codereview.chromium.org/9243011/diff/2001/frog/leg/warnings.dart
File frog/leg/warnings.dart (right):

http://codereview.chromium.org/9243011/diff/2001/frog/leg/warnings.dart#newco...
frog/leg/warnings.dart:64: 'only call to \'this\' or \'super\' constructor
allowed');
On 2012/01/18 15:22:14, floitsch wrote:
> you could avoid the escapes by using " to delimit the string.

Done.

http://codereview.chromium.org/9243011/diff/2001/frog/leg/warnings.dart#newco...
frog/leg/warnings.dart:70: "'Object' cannot have a superinitializer");
On 2012/01/18 15:22:14, floitsch wrote:
> super initializer

Done.

[ngeoffray, 2012-01-19 08:56:11]

Lots of comments :)

http://codereview.chromium.org/9243011/diff/5002/frog/leg/elements/elements.dart
File frog/leg/elements/elements.dart (right):

http://codereview.chromium.org/9243011/diff/5002/frog/leg/elements/elements.d...
frog/leg/elements/elements.dart:358: // TODO(ngeoffray): Implement these.
Remove the TODO?

http://codereview.chromium.org/9243011/diff/5002/frog/leg/resolver.dart
File frog/leg/resolver.dart (right):

http://codereview.chromium.org/9243011/diff/5002/frog/leg/resolver.dart#newco...
frog/leg/resolver.dart:88: Map<SourceString, Node> initialized;
Make initialized final?

http://codereview.chromium.org/9243011/diff/5002/frog/leg/resolver.dart#newco...
frog/leg/resolver.dart:89: Node initializerOrSuper;
Instead of having initializerOrSuper, I suggest keeping the initializers node
around. Then, if you see a redirector consturctor, make sure the length of its
list of nodes is 1.

http://codereview.chromium.org/9243011/diff/5002/frog/leg/resolver.dart#newco...
frog/leg/resolver.dart:95: Universe get universe() => visitor.compiler.universe;
Unused?

http://codereview.chromium.org/9243011/diff/5002/frog/leg/resolver.dart#newco...
frog/leg/resolver.dart:134: error(init, MessageKind.DUPLICATE_INITIALIZER,
[name]);
Why error + warning? Shouldn't it be just one of those? (don't have the spec
available, so I don't know which one)

http://codereview.chromium.org/9243011/diff/5002/frog/leg/resolver.dart#newco...
frog/leg/resolver.dart:166: if (constructor.name === Types.OBJECT) {
Shouldn't that just be lookupTarget === Types.OBJECT ?

http://codereview.chromium.org/9243011/diff/5002/frog/leg/resolver.dart#newco...
frog/leg/resolver.dart:167: error(call,
MessageKind.SUPER_INITIALIZER_IN_OBJECT);
Should you return here? Otherwise you would do a lookup of something in Object,
which is maybe not what you want, as you will get another similar error message.

http://codereview.chromium.org/9243011/diff/5002/frog/leg/resolver.dart#newco...
frog/leg/resolver.dart:191: error(call, MessageKind.CANNOT_RESOLVE,
["constructor $name"]);
Maybe add a CANNOT_RESOLVE_CONSTRUCTOR, to avoid having the 'constructor' in the
argument?

http://codereview.chromium.org/9243011/diff/5002/frog/leg/resolver.dart#newco...
frog/leg/resolver.dart:192: } else {
The parameters may already there, so I don't think you should call parseNode
here. Instead, I would add a parameterCount method on a FunctionElement and let
it resolve its signature:

int parameterCount(Compiler compiler) {
  if (parameters == null) compiler.resolveSignature(element);
  return parameters.length;
}

http://codereview.chromium.org/9243011/diff/5002/frog/leg/resolver.dart#newco...
frog/leg/resolver.dart:201: visitor.compiler.enqueue(new
WorkItem.toCompile(target));
You should leave the codegen deciding if it should be enqueued or not.

http://codereview.chromium.org/9243011/diff/5002/frog/leg/resolver.dart#newco...
frog/leg/resolver.dart:214: // TODO(karlklose): implement initializer
parameters.
Please add an unimplemented where this is not handled. It looks to me that
resolveFieldInitializer will just report a random error.

http://codereview.chromium.org/9243011/diff/5002/frog/leg/resolver.dart#newco...
frog/leg/resolver.dart:297: if (node.isThis() || node.isSuper()) {
This is already handled line 302.

http://codereview.chromium.org/9243011/diff/5002/frog/leg/resolver.dart#newco...
frog/leg/resolver.dart:318: // denote named parameters or library prefixes.
named parameters -> named constructor?

http://codereview.chromium.org/9243011/diff/5002/frog/leg/resolver.dart#newco...
frog/leg/resolver.dart:436: visitFunctionExpression(FunctionExpression node) {
I don't think you can be here for a constructor. This method should just be for
closures, as you're defining a new element. The parser takes care of defining an
element for constructors.

http://codereview.chromium.org/9243011/diff/5002/frog/leg/resolver.dart#newco...
frog/leg/resolver.dart:649: FunctionExpression fun =
constructor.parseNode(compiler, compiler);
Same comment for resolveSignature.

http://codereview.chromium.org/9243011/diff/5002/frog/leg/scanner/class_eleme...
File frog/leg/scanner/class_element_parser.dart (right):

http://codereview.chromium.org/9243011/diff/5002/frog/leg/scanner/class_eleme...
frog/leg/scanner/class_element_parser.dart:42: bool isConstructorName(Node
nameNode) {
Why renaming it to isConstructorName? isConstructor looked ok to me.

http://codereview.chromium.org/9243011/diff/5002/frog/leg/scanner/class_eleme...
frog/leg/scanner/class_element_parser.dart:68: name = new
SourceString('$className.$constructorName');
Since you are putting constructors in a different map, I don't think you need to
'mangle' the constructor's name. For named constructors, you can use their
names, and for the unnamed constructor, you could use the empty string.

Also, I have seen this 'mangling' at three places, so it would automatically
remove this code duplication.

http://codereview.chromium.org/9243011/diff/5002/frog/leg/tree/nodes.dart
File frog/leg/tree/nodes.dart (right):

http://codereview.chromium.org/9243011/diff/5002/frog/leg/tree/nodes.dart#new...
frog/leg/tree/nodes.dart:229: bool get isSuperConstructorCall() {
I don't really like these methods being here, as they would lie in case you are
inside a method. I think you should put them as helpers in the resolver.

http://codereview.chromium.org/9243011/diff/5002/frog/leg/tree/nodes.dart#new...
frog/leg/tree/nodes.dart:251: selector.asIdentifier() !== null;
I think you're allowed to do super[2]. Checking if the selector is an identifier
would return false here, I believe.

http://codereview.chromium.org/9243011/diff/5002/frog/leg/tree/nodes.dart#new...
frog/leg/tree/nodes.dart:543: int _cachedParameterCount;
Like I said in the resolver, I think this belongs to FunctionElement.

http://codereview.chromium.org/9243011/diff/5002/frog/tests/leg/src/ResolverT...
File frog/tests/leg/src/ResolverTest.dart (right):

http://codereview.chromium.org/9243011/diff/5002/frog/tests/leg/src/ResolverT...
frog/tests/leg/src/ResolverTest.dart:548: [],
[MessageKind.DUPLICATE_SUPER_INITIALIZER]);
Could you also add tests for class Object?

http://codereview.chromium.org/9243011/diff/5002/tests/language/language-leg....
File tests/language/language-leg.status (right):

http://codereview.chromium.org/9243011/diff/5002/tests/language/language-leg....
tests/language/language-leg.status:50: Constructor2Test: Fail
order.

http://codereview.chromium.org/9243011/diff/5002/tests/language/language-leg....
tests/language/language-leg.status:61: ConstructorReturnTest: Fail
ditto

http://codereview.chromium.org/9243011/diff/5002/tests/language/language-leg....
tests/language/language-leg.status:64: ConstructorReturnWithInitNegativeTest:
Fail
ditto

[karlklose, 2012-01-19 13:51:24]

A lot of changes :-)

http://codereview.chromium.org/9243011/diff/5002/frog/leg/elements/elements.dart
File frog/leg/elements/elements.dart (right):

http://codereview.chromium.org/9243011/diff/5002/frog/leg/elements/elements.d...
frog/leg/elements/elements.dart:358: // TODO(ngeoffray): Implement these.
On 2012/01/19 08:56:12, ngeoffray wrote:
> Remove the TODO?

Done.

http://codereview.chromium.org/9243011/diff/5002/frog/leg/resolver.dart
File frog/leg/resolver.dart (right):

http://codereview.chromium.org/9243011/diff/5002/frog/leg/resolver.dart#newco...
frog/leg/resolver.dart:88: Map<SourceString, Node> initialized;
On 2012/01/19 08:56:12, ngeoffray wrote:
> Make initialized final?

Done.

http://codereview.chromium.org/9243011/diff/5002/frog/leg/resolver.dart#newco...
frog/leg/resolver.dart:89: Node initializerOrSuper;
On 2012/01/19 08:56:12, ngeoffray wrote:
> Instead of having initializerOrSuper, I suggest keeping the initializers node
> around. Then, if you see a redirector consturctor, make sure the length of its
> list of nodes is 1.

Done.

http://codereview.chromium.org/9243011/diff/5002/frog/leg/resolver.dart#newco...
frog/leg/resolver.dart:95: Universe get universe() => visitor.compiler.universe;
Done, removed.

http://codereview.chromium.org/9243011/diff/5002/frog/leg/resolver.dart#newco...
frog/leg/resolver.dart:134: error(init, MessageKind.DUPLICATE_INITIALIZER,
[name]);
The warning gives additional feedback.

http://codereview.chromium.org/9243011/diff/5002/frog/leg/resolver.dart#newco...
frog/leg/resolver.dart:166: if (constructor.name === Types.OBJECT) {
On 2012/01/19 08:56:12, ngeoffray wrote:
> Shouldn't that just be lookupTarget === Types.OBJECT ?

Done.

http://codereview.chromium.org/9243011/diff/5002/frog/leg/resolver.dart#newco...
frog/leg/resolver.dart:167: error(call,
MessageKind.SUPER_INITIALIZER_IN_OBJECT);
I skipped the lookup in this case but still resolve the arguments.

http://codereview.chromium.org/9243011/diff/5002/frog/leg/resolver.dart#newco...
frog/leg/resolver.dart:191: error(call, MessageKind.CANNOT_RESOLVE,
["constructor $name"]);
On 2012/01/19 08:56:12, ngeoffray wrote:
> Maybe add a CANNOT_RESOLVE_CONSTRUCTOR, to avoid having the 'constructor' in
the
> argument?

Done.

http://codereview.chromium.org/9243011/diff/5002/frog/leg/resolver.dart#newco...
frog/leg/resolver.dart:192: } else {
On 2012/01/19 08:56:12, ngeoffray wrote:
> The parameters may already there, so I don't think you should call parseNode
> here. Instead, I would add a parameterCount method on a FunctionElement and
let
> it resolve its signature:
> 
> int parameterCount(Compiler compiler) {
>   if (parameters == null) compiler.resolveSignature(element);
>   return parameters.length;
> }

Done.

http://codereview.chromium.org/9243011/diff/5002/frog/leg/resolver.dart#newco...
frog/leg/resolver.dart:201: visitor.compiler.enqueue(new
WorkItem.toCompile(target));
On 2012/01/19 08:56:12, ngeoffray wrote:
> You should leave the codegen deciding if it should be enqueued or not.

Done.

http://codereview.chromium.org/9243011/diff/5002/frog/leg/resolver.dart#newco...
frog/leg/resolver.dart:214: // TODO(karlklose): implement initializer
parameters.
This is not unimplemented here, it was only a note that we have to take care of
the names of initializer parameters when checking that all finals are
initialized. I removed the TODO.

http://codereview.chromium.org/9243011/diff/5002/frog/leg/resolver.dart#newco...
frog/leg/resolver.dart:297: if (node.isThis() || node.isSuper()) {
On 2012/01/19 08:56:12, ngeoffray wrote:
> This is already handled line 302.

Done.

http://codereview.chromium.org/9243011/diff/5002/frog/leg/resolver.dart#newco...
frog/leg/resolver.dart:318: // denote named parameters or library prefixes.
On 2012/01/19 08:56:12, ngeoffray wrote:
> named parameters -> named constructor?

Done.

http://codereview.chromium.org/9243011/diff/5002/frog/leg/resolver.dart#newco...
frog/leg/resolver.dart:436: visitFunctionExpression(FunctionExpression node) {
On 2012/01/19 08:56:12, ngeoffray wrote:
> I don't think you can be here for a constructor. This method should just be
for
> closures, as you're defining a new element. The parser takes care of defining
an
> element for constructors.

Done.

http://codereview.chromium.org/9243011/diff/5002/frog/leg/resolver.dart#newco...
frog/leg/resolver.dart:649: FunctionExpression fun =
constructor.parseNode(compiler, compiler);
On 2012/01/19 08:56:12, ngeoffray wrote:
> Same comment for resolveSignature.

Done.

http://codereview.chromium.org/9243011/diff/5002/frog/leg/scanner/class_eleme...
File frog/leg/scanner/class_element_parser.dart (right):

http://codereview.chromium.org/9243011/diff/5002/frog/leg/scanner/class_eleme...
frog/leg/scanner/class_element_parser.dart:42: bool isConstructorName(Node
nameNode) {
Because it operates on a 'name'-node, not the constructor itself.

http://codereview.chromium.org/9243011/diff/5002/frog/leg/scanner/class_eleme...
frog/leg/scanner/class_element_parser.dart:68: name = new
SourceString('$className.$constructorName');
The map contains factories, too.

http://codereview.chromium.org/9243011/diff/5002/frog/leg/tree/nodes.dart
File frog/leg/tree/nodes.dart (right):

http://codereview.chromium.org/9243011/diff/5002/frog/leg/tree/nodes.dart#new...
frog/leg/tree/nodes.dart:229: bool get isSuperConstructorCall() {
On 2012/01/19 08:56:12, ngeoffray wrote:
> I don't really like these methods being here, as they would lie in case you
are
> inside a method. I think you should put them as helpers in the resolver.

Done.

http://codereview.chromium.org/9243011/diff/5002/frog/leg/tree/nodes.dart#new...
frog/leg/tree/nodes.dart:251: selector.asIdentifier() !== null;
On 2012/01/19 08:56:12, ngeoffray wrote:
> I think you're allowed to do super[2]. Checking if the selector is an
identifier
> would return false here, I believe.

Done.

http://codereview.chromium.org/9243011/diff/5002/frog/leg/tree/nodes.dart#new...
frog/leg/tree/nodes.dart:543: int _cachedParameterCount;
On 2012/01/19 08:56:12, ngeoffray wrote:
> Like I said in the resolver, I think this belongs to FunctionElement.

Done.

http://codereview.chromium.org/9243011/diff/5002/frog/tests/leg/src/ResolverT...
File frog/tests/leg/src/ResolverTest.dart (right):

http://codereview.chromium.org/9243011/diff/5002/frog/tests/leg/src/ResolverT...
frog/tests/leg/src/ResolverTest.dart:548: [],
[MessageKind.DUPLICATE_SUPER_INITIALIZER]);
On 2012/01/19 08:56:12, ngeoffray wrote:
> Could you also add tests for class Object?

Done.

http://codereview.chromium.org/9243011/diff/5002/tests/language/language-leg....
File tests/language/language-leg.status (right):

http://codereview.chromium.org/9243011/diff/5002/tests/language/language-leg....
tests/language/language-leg.status:50: Constructor2Test: Fail
On 2012/01/19 08:56:12, ngeoffray wrote:
> order.

Done.

http://codereview.chromium.org/9243011/diff/5002/tests/language/language-leg....
tests/language/language-leg.status:61: ConstructorReturnTest: Fail
On 2012/01/19 08:56:12, ngeoffray wrote:
> ditto

Done.

http://codereview.chromium.org/9243011/diff/5002/tests/language/language-leg....
tests/language/language-leg.status:64: ConstructorReturnWithInitNegativeTest:
Fail
On 2012/01/19 08:56:12, ngeoffray wrote:
> ditto

Done.

[ngeoffray, 2012-01-19 14:36:33]

http://codereview.chromium.org/9243011/diff/5002/frog/leg/resolver.dart
File frog/leg/resolver.dart (right):

http://codereview.chromium.org/9243011/diff/5002/frog/leg/resolver.dart#newco...
frog/leg/resolver.dart:134: error(init, MessageKind.DUPLICATE_INITIALIZER,
[name]);
On 2012/01/19 13:51:24, karlklose wrote:
> The warning gives additional feedback.

So why not putting all the feedback in a single error?

http://codereview.chromium.org/9243011/diff/5002/frog/leg/resolver.dart#newco...
frog/leg/resolver.dart:167: error(call,
MessageKind.SUPER_INITIALIZER_IN_OBJECT);
On 2012/01/19 13:51:24, karlklose wrote:
> I skipped the lookup in this case but still resolve the arguments.

Good point.

http://codereview.chromium.org/9243011/diff/5002/frog/leg/resolver.dart#newco...
frog/leg/resolver.dart:214: // TODO(karlklose): implement initializer
parameters.
On 2012/01/19 13:51:24, karlklose wrote:
> This is not unimplemented here, it was only a note that we have to take care
of
> the names of initializer parameters when checking that all finals are
> initialized. I removed the TODO.

I see. Thanks for the explanation.

http://codereview.chromium.org/9243011/diff/5002/frog/leg/scanner/class_eleme...
File frog/leg/scanner/class_element_parser.dart (right):

http://codereview.chromium.org/9243011/diff/5002/frog/leg/scanner/class_eleme...
frog/leg/scanner/class_element_parser.dart:42: bool isConstructorName(Node
nameNode) {
On 2012/01/19 13:51:24, karlklose wrote:
> Because it operates on a 'name'-node, not the constructor itself.

OK.

http://codereview.chromium.org/9243011/diff/5002/frog/leg/scanner/class_eleme...
frog/leg/scanner/class_element_parser.dart:68: name = new
SourceString('$className.$constructorName');
On 2012/01/19 13:51:24, karlklose wrote:
> The map contains factories, too.

The 'constructor' map right? That's no problem, generative constructors and
factories share the same namespace. So I still think my comment is valid.

http://codereview.chromium.org/9243011/diff/11001/frog/leg/ssa/builder.dart
File frog/leg/ssa/builder.dart (right):

http://codereview.chromium.org/9243011/diff/11001/frog/leg/ssa/builder.dart#n...
frog/leg/ssa/builder.dart:201: Send superConstructor;
Why adding these?

[karlklose, 2012-01-19 16:06:26]

> The 'constructor' map right? That's no problem, generative > constructors and
> factories share the same namespace. So I still think my 
> comment is valid.
As discussed offline, we need to look-up constructors and factories with their
full name, because a class name contain factories with the same name for
different interfaces.

http://codereview.chromium.org/9243011/diff/11001/frog/leg/ssa/builder.dart
File frog/leg/ssa/builder.dart (right):

http://codereview.chromium.org/9243011/diff/11001/frog/leg/ssa/builder.dart#n...
frog/leg/ssa/builder.dart:201: Send superConstructor;
Removed.

[ngeoffray, 2012-01-20 13:54:17]

LGTM!

http://codereview.chromium.org/9243011/diff/7013/frog/leg/elements/elements.dart
File frog/leg/elements/elements.dart (right):

http://codereview.chromium.org/9243011/diff/7013/frog/leg/elements/elements.d...
frog/leg/elements/elements.dart:363: [SourceString constructor = const
SourceString(''),
constructor -> constructorName?

http://codereview.chromium.org/9243011/diff/7013/frog/leg/elements/elements.d...
frog/leg/elements/elements.dart:375: result = noMatch(lookupLocalMember(name));
Shouldn't you check for constructorName if it is not the unnamed constructor?

http://codereview.chromium.org/9243011/diff/7013/frog/leg/scanner/class_eleme...
File frog/leg/scanner/class_element_parser.dart (right):

http://codereview.chromium.org/9243011/diff/7013/frog/leg/scanner/class_eleme...
frog/leg/scanner/class_element_parser.dart:63: if (method.name.asSend() != null)
{
I would put method.name in a variable (also used lines 61, 65, 66, 71).

[karlklose, 2012-01-20 14:11:36]

Thanks for the review, Nicolas!

http://codereview.chromium.org/9243011/diff/7013/frog/leg/elements/elements.dart
File frog/leg/elements/elements.dart (right):

http://codereview.chromium.org/9243011/diff/7013/frog/leg/elements/elements.d...
frog/leg/elements/elements.dart:363: [SourceString constructor = const
SourceString(''),
On 2012/01/20 13:54:17, ngeoffray wrote:
> constructor -> constructorName?

Done.

http://codereview.chromium.org/9243011/diff/7013/frog/leg/elements/elements.d...
frog/leg/elements/elements.dart:375: result = noMatch(lookupLocalMember(name));
On 2012/01/20 13:54:17, ngeoffray wrote:
> Shouldn't you check for constructorName if it is not the unnamed constructor?

Done.

http://codereview.chromium.org/9243011/diff/7013/frog/leg/scanner/class_eleme...
File frog/leg/scanner/class_element_parser.dart (right):

http://codereview.chromium.org/9243011/diff/7013/frog/leg/scanner/class_eleme...
frog/leg/scanner/class_element_parser.dart:63: if (method.name.asSend() != null)
{
On 2012/01/20 13:54:17, ngeoffray wrote:
> I would put method.name in a variable (also used lines 61, 65, 66, 71).

Done.

[ahe, 2012-02-15 07:31:15]

https://chromiumcodereview.appspot.com/9243011/diff/7013/frog/leg/scanner/cla...
File frog/leg/scanner/class_element_parser.dart (right):

https://chromiumcodereview.appspot.com/9243011/diff/7013/frog/leg/scanner/cla...
frog/leg/scanner/class_element_parser.dart:69: name = new
SourceString('$className.$constructorName');
The scanner and parser generally takes care to avoid concatenating strings. This
innocuous looking expression could be equivalent to something like this:

String x = new String.fromCharCodes(new Utf8Decoder(className.bytes,
className.offset, className.length).decodeRest());
String y = new String.fromCharCodes(new Utf8Decoder(constructorName.bytes,
constructorName.offset, constructorName.length).decodeRest());
StringBuffer sb = new StringBuffer();
sb.add(x);
sb.add(y);
name = new SourceString(sb.toString());

This means constructing seven temporary objects (the UTF8 decoder returns a
List<int>). Also, the bytes of each source string is copied three times.

Right now, I'm seeing a 14% improvement from using ByteArray on
vm_scanner_bench. However, when I try to use ByteArray in Leg, scanner
performance regress. So we're somehow losing more than 14% performance in
class_element_parser. I'm not sure that this particular change is responsible
for this regression, but it underlines the importance of being very careful
about these things.

Please don't forget to include me on changes to the scanner in the future.

[ahe, 2012-02-15 09:25:00]

https://chromiumcodereview.appspot.com/9243011/diff/7013/frog/leg/scanner/cla...
File frog/leg/scanner/class_element_parser.dart (right):

https://chromiumcodereview.appspot.com/9243011/diff/7013/frog/leg/scanner/cla...
frog/leg/scanner/class_element_parser.dart:69: name = new
SourceString('$className.$constructorName');
On 2012/02/15 07:31:15, ahe wrote:
> Right now, I'm seeing a 14% improvement from using ByteArray on
> vm_scanner_bench. However, when I try to use ByteArray in Leg, scanner
> performance regress. So we're somehow losing more than 14% performance in
> class_element_parser. I'm not sure that this particular change is responsible
> for this regression, but it underlines the importance of being very careful
> about these things.

I made some additional instrumentation and I can completely clear this
particular change of any adverse effects on the scanner performance of
hello_world.dart.