fixes #193, factory constructors as static methods

fixes #193, factory constructors as static methods

R=vsm@google.com

Committed: https://github.com/dart-lang/dev_compiler/commit/4947b85e517c50d5039bc66e0539ec07eeb1b477

[Jennifer Messerly, 2015-05-29 19:54:52]

jmesserly@google.com changed reviewers:
+ vsm@google.com

[Jennifer Messerly, 2015-05-29 19:54:53]

https://codereview.chromium.org/1156993015/diff/1/lib/src/codegen/js_codegen....
File lib/src/codegen/js_codegen.dart (right):

https://codereview.chromium.org/1156993015/diff/1/lib/src/codegen/js_codegen....
lib/src/codegen/js_codegen.dart:678: // Wacky factory redirecting constructors:
factory Foo.q(x, y) = Bar.baz;
this code moved from _emitConstructorBody, which was kind of a strange place for
it.

[vsm, 2015-05-29 21:08:58]

lgtm

https://codereview.chromium.org/1156993015/diff/1/.gitignore
File .gitignore (right):

https://codereview.chromium.org/1156993015/diff/1/.gitignore#newcode18
.gitignore:18: test/generated_sdk/sdk_errors.txt
thanks :-)

https://codereview.chromium.org/1156993015/diff/1/lib/runtime/dart_runtime.js
File lib/runtime/dart_runtime.js (right):

https://codereview.chromium.org/1156993015/diff/1/lib/runtime/dart_runtime.js...
lib/runtime/dart_runtime.js:913: let map = collection.LinkedHashMap.new();
Hmm, it's too bad the JS user has to now keep track of whether it's a factory or
not as well.  Not sure there's a clean way around it though.

[Jennifer Messerly, 2015-05-29 21:32:17]

https://codereview.chromium.org/1156993015/diff/1/lib/runtime/dart_runtime.js
File lib/runtime/dart_runtime.js (right):

https://codereview.chromium.org/1156993015/diff/1/lib/runtime/dart_runtime.js...
lib/runtime/dart_runtime.js:913: let map = collection.LinkedHashMap.new();
On 2015/05/29 21:08:58, vsm wrote:
> Hmm, it's too bad the JS user has to now keep track of whether it's a factory
or
> not as well.  Not sure there's a clean way around it though.  

yeah, seems kind of okay because:

* the only way to represent this pattern in JS is with static methods.
* in JS they look like static methods if you go to definition.
* in Dart, they're clearly documented as "factory" and function more like static
methods anyway.

It's not perfect though, agreed.

Here's another option: disallow returning "null" from a factory method, in
essence assert the factory returns a non-nullable type. This would need an extra
runtime check though. Then we'd only need to do this pattern for primitives, to
avoid boxing.

would you prefer that option?

[vsm, 2015-05-29 21:45:42]

On 2015/05/29 21:32:17, John Messerly wrote:
> https://codereview.chromium.org/1156993015/diff/1/lib/runtime/dart_runtime.js
> File lib/runtime/dart_runtime.js (right):
> 
>
https://codereview.chromium.org/1156993015/diff/1/lib/runtime/dart_runtime.js...
> lib/runtime/dart_runtime.js:913: let map = collection.LinkedHashMap.new();
> On 2015/05/29 21:08:58, vsm wrote:
> > Hmm, it's too bad the JS user has to now keep track of whether it's a
factory
> or
> > not as well.  Not sure there's a clean way around it though.  
> 
> yeah, seems kind of okay because:
> 
> * the only way to represent this pattern in JS is with static methods.
> * in JS they look like static methods if you go to definition.
> * in Dart, they're clearly documented as "factory" and function more like
static
> methods anyway.
> 
> It's not perfect though, agreed.
> 
> Here's another option: disallow returning "null" from a factory method, in
> essence assert the factory returns a non-nullable type. This would need an
extra
> runtime check though. Then we'd only need to do this pattern for primitives,
to
> avoid boxing.
> 
> would you prefer that option?

I'd lean toward your current approach.  It also does get rid of the extra
allocation, which is nice.

[Jennifer Messerly, 2015-05-29 21:52:15]

Committed patchset #1 (id:1) manually as
4947b85e517c50d5039bc66e0539ec07eeb1b477 (presubmit successful).