Support factory methods.

Support factory methods.
Committed: https://code.google.com/p/dart/source/detail?r=2759

[kasperl, 2011-12-22 11:34:41]

LGTM. Not sure about the super long GENERATIVE_CONSTRUCTOR name but maybe that's
okay.

http://codereview.chromium.org/9027002/diff/10/frog/leg/lib/core.dart
File frog/leg/lib/core.dart (right):

http://codereview.chromium.org/9027002/diff/10/frog/leg/lib/core.dart#newcode250
frog/leg/lib/core.dart:250: return JS(@"new Array($0)", n);
Should we be checking that i is an int here (at least a number)?

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

http://codereview.chromium.org/9027002/diff/10/frog/leg/resolver.dart#newcode123
frog/leg/resolver.dart:123: || element.kind ==
ElementKind.GENERATIVE_CONSTRUCTOR,
element.isGenerativeConstructor would be somewhat shorter.

http://codereview.chromium.org/9027002/diff/10/frog/tests/leg_only/src/Factor...
File frog/tests/leg_only/src/FactoryTest.dart (right):

http://codereview.chromium.org/9027002/diff/10/frog/tests/leg_only/src/Factor...
frog/tests/leg_only/src/FactoryTest.dart:5: void expectEquals(var expected, var
actual) {
How hard would it be to move this to something ala Expect.equals soon? Do we
have support for static method invocations on classes?

[ngeoffray, 2011-12-22 13:07:01]

PTAL, especially for the isInt check.

About GENERATIVE_CONSTRUCTOR, I changed it this way because the spec defines a
constructor as a generative constructor or a factory method. Do you prefer
GEN_CONSTRUCTOR?

http://codereview.chromium.org/9027002/diff/10/frog/leg/lib/core.dart
File frog/leg/lib/core.dart (right):

http://codereview.chromium.org/9027002/diff/10/frog/leg/lib/core.dart#newcode250
frog/leg/lib/core.dart:250: return JS(@"new Array($0)", n);
On 2011/12/22 11:34:42, kasperl wrote:
> Should we be checking that i is an int here (at least a number)?

Done.

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

http://codereview.chromium.org/9027002/diff/10/frog/leg/resolver.dart#newcode123
frog/leg/resolver.dart:123: || element.kind ==
ElementKind.GENERATIVE_CONSTRUCTOR,
On 2011/12/22 11:34:42, kasperl wrote:
> element.isGenerativeConstructor would be somewhat shorter.

Done.

http://codereview.chromium.org/9027002/diff/10/frog/tests/leg_only/src/Factor...
File frog/tests/leg_only/src/FactoryTest.dart (right):

http://codereview.chromium.org/9027002/diff/10/frog/tests/leg_only/src/Factor...
frog/tests/leg_only/src/FactoryTest.dart:5: void expectEquals(var expected, var
actual) {
On 2011/12/22 11:34:42, kasperl wrote:
> How hard would it be to move this to something ala Expect.equals soon? Do we
> have support for static method invocations on classes?

Looks like you just did it :)

[kasperl, 2011-12-22 13:08:52]

LGTM.

[ahe, 2011-12-22 13:32:39]

LGTM!

I was surprised how simple this change was. Once you filter out the
GENERATIVE_CONSTRUCTOR changes it really isn't much.

Cheers,
Peter

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

http://codereview.chromium.org/9027002/diff/3013/frog/leg/elements/elements.d...
frog/leg/elements/elements.dart:32: static final ElementKind
GENERATIVE_CONSTRUCTOR =
I like using terminology from the specification.

http://codereview.chromium.org/9027002/diff/3013/frog/leg/lib/core.dart
File frog/leg/lib/core.dart (right):

http://codereview.chromium.org/9027002/diff/3013/frog/leg/lib/core.dart#newco...
frog/leg/lib/core.dart:256: if (!isInt(n)) throw "Invalid length";
What about negative sizes?

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

http://codereview.chromium.org/9027002/diff/3013/frog/leg/resolver.dart#newco...
frog/leg/resolver.dart:123: || element.isGenerativeConstructor(),
Much better!

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

http://codereview.chromium.org/9027002/diff/3013/frog/leg/scanner/class_eleme...
frog/leg/scanner/class_element_parser.dart:65: Identifier name =
method.returnType.typeName;
You need to change this now. The return type is always null now.

I think you need to cancel if the name is a Send.

[ngeoffray, 2011-12-22 13:50:22]

Thanks Peter and Kasper!

http://codereview.chromium.org/9027002/diff/3013/frog/leg/lib/core.dart
File frog/leg/lib/core.dart (right):

http://codereview.chromium.org/9027002/diff/3013/frog/leg/lib/core.dart#newco...
frog/leg/lib/core.dart:256: if (!isInt(n)) throw "Invalid length";
On 2011/12/22 13:32:39, ahe wrote:
> What about negative sizes?

Done.

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

http://codereview.chromium.org/9027002/diff/3013/frog/leg/scanner/class_eleme...
frog/leg/scanner/class_element_parser.dart:65: Identifier name =
method.returnType.typeName;
On 2011/12/22 13:32:39, ahe wrote:
> You need to change this now. The return type is always null now.
> 
> I think you need to cancel if the name is a Send.

Done.

[ahe, 2011-12-22 14:18:05]

A minor follow-up question.

http://codereview.chromium.org/9027002/diff/5008/frog/leg/lib/core.dart
File frog/leg/lib/core.dart (right):

http://codereview.chromium.org/9027002/diff/5008/frog/leg/lib/core.dart#newco...
frog/leg/lib/core.dart:246: return JS(@"($0 | 0) === $1", v, v);
Why not:

JS(@"($0 | 0) === $0", v);

[floitsch, 2011-12-22 14:36:18]

http://codereview.chromium.org/9027002/diff/5008/frog/leg/lib/core.dart
File frog/leg/lib/core.dart (right):

http://codereview.chromium.org/9027002/diff/5008/frog/leg/lib/core.dart#newco...
frog/leg/lib/core.dart:246: return JS(@"($0 | 0) === $1", v, v);
On 2011/12/22 14:18:05, ahe wrote:
> Why not:
> 
> JS(@"($0 | 0) === $0", v);

I asked him to do it this way. This way generating v at use-site would still be
legal for arguments to JS. If 'v' is an expression with side-effects it would
not be marked since there are two uses.
We have another guard against this, but I would actually prefer to get rid of
the other guard, since it always disallows generate-at-use-site for JS.

[ahe, 2011-12-22 14:41:21]

http://codereview.chromium.org/9027002/diff/5008/frog/leg/lib/core.dart
File frog/leg/lib/core.dart (right):

http://codereview.chromium.org/9027002/diff/5008/frog/leg/lib/core.dart#newco...
frog/leg/lib/core.dart:246: return JS(@"($0 | 0) === $1", v, v);
On 2011/12/22 14:36:19, floitsch wrote:
> On 2011/12/22 14:18:05, ahe wrote:
> > Why not:
> > 
> > JS(@"($0 | 0) === $0", v);
> 
> I asked him to do it this way. This way generating v at use-site would still
be
> legal for arguments to JS. If 'v' is an expression with side-effects it would
> not be marked since there are two uses.
> We have another guard against this, but I would actually prefer to get rid of
> the other guard, since it always disallows generate-at-use-site for JS.

I'm not sure I understand. Are you saying that you requested this one:

a: JS(@"($0 | 0) === $0", v);

or this one:

b: JS(@"($0 | 0) === $1", v, v);

[floitsch, 2011-12-22 15:13:21]

http://codereview.chromium.org/9027002/diff/5008/frog/leg/lib/core.dart
File frog/leg/lib/core.dart (right):

http://codereview.chromium.org/9027002/diff/5008/frog/leg/lib/core.dart#newco...
frog/leg/lib/core.dart:246: return JS(@"($0 | 0) === $1", v, v);
On 2011/12/22 14:41:22, ahe wrote:
> On 2011/12/22 14:36:19, floitsch wrote:
> > On 2011/12/22 14:18:05, ahe wrote:
> > > Why not:
> > > 
> > > JS(@"($0 | 0) === $0", v);
> > 
> > I asked him to do it this way. This way generating v at use-site would still
> be
> > legal for arguments to JS. If 'v' is an expression with side-effects it
would
> > not be marked since there are two uses.
> > We have another guard against this, but I would actually prefer to get rid
of
> > the other guard, since it always disallows generate-at-use-site for JS.
> 
> I'm not sure I understand. Are you saying that you requested this one:
> 
> a: JS(@"($0 | 0) === $0", v);
> 
> or this one:
> 
> b: JS(@"($0 | 0) === $1", v, v);
sorry for the confusion. I asked him to do b. I find it cleaner if the compiler
knows how often the variable is used.