Expands on previous function RTT to support named/optional parameters.

Expands on previous function RTT to support named/optional parameters.

Issues encountered:
http://code.google.com/p/dart/issues/detail?id=847
http://code.google.com/p/dart/issues/detail?id=848


BUG=
TEST=

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

[codefu, 2011-12-14 16:12:09]

http://codereview.chromium.org/8913006/diff/1/compiler/java/com/google/dart/c...
File compiler/java/com/google/dart/compiler/backend/js/RuntimeTypeInjector.java
(right):

http://codereview.chromium.org/8913006/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/backend/js/RuntimeTypeInjector.java:1289:
// TODO: We need a more detailed RTT than just a generic function.
Issue 847 clarification.

http://codereview.chromium.org/8913006/diff/1/compiler/lib/implementation/arr...
File compiler/lib/implementation/array.dart (right):

http://codereview.chromium.org/8913006/diff/1/compiler/lib/implementation/arr...
compiler/lib/implementation/array.dart:7: if (other == null) {
When testing optimized co19/LibTest/core/List/List.from/List/from/A03/t01, this
function is changed to static and the NPE generated by accessing "other" doesn't
happen.

http://codereview.chromium.org/8913006/diff/1/compiler/lib/implementation/arr...
compiler/lib/implementation/array.dart:180: if (elements == null) {
dito above, test: co19/LibTest/core/List/addAll/List/addAll/A03/t01

http://codereview.chromium.org/8913006/diff/1/compiler/lib/implementation/str...
File compiler/lib/implementation/string_buffer.dart (right):

http://codereview.chromium.org/8913006/diff/1/compiler/lib/implementation/str...
compiler/lib/implementation/string_buffer.dart:45: if (objects == null) {
Same optimization error;
co19/LibTest/core/StringBuffer/addAll/StringBuffer/addAll/A03/t01

[mmendez, 2011-12-14 20:48:13]

lgtm with nits

http://codereview.chromium.org/8913006/diff/1/compiler/java/com/google/dart/c...
File
compiler/java/com/google/dart/compiler/backend/js/GenerateJavascriptAST.java
(right):

http://codereview.chromium.org/8913006/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/backend/js/GenerateJavascriptAST.java:609:
JsNameRef mangledRttMethod = rtt.getRTTLookupMethodName(methodElement);
Nit: maybe mangledRttMethodRef would be a better name?

http://codereview.chromium.org/8913006/diff/1/compiler/java/com/google/dart/c...
File compiler/java/com/google/dart/compiler/backend/js/RuntimeTypeInjector.java
(left):

http://codereview.chromium.org/8913006/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/backend/js/RuntimeTypeInjector.java:254:
} else if (hasTypeParams) {
Nit: Alternatively, you could still only pass null only if type parameters are
at play and named.  Otherwise, you could not emit the nulls.  Small code
savings.

http://codereview.chromium.org/8913006/diff/1/compiler/java/com/google/dart/c...
File compiler/java/com/google/dart/compiler/backend/js/RuntimeTypeInjector.java
(right):

http://codereview.chromium.org/8913006/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/backend/js/RuntimeTypeInjector.java:525:
return newInvocation(newQualifiedNameRef("RTT.dynamicType.$lookupRTT"));
Nit: not part of your patch, but you could refactor the string into a constant.

http://codereview.chromium.org/8913006/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/backend/js/RuntimeTypeInjector.java:625:
return elementInvoke;
Nit: could assert not null here.

http://codereview.chromium.org/8913006/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/backend/js/RuntimeTypeInjector.java:671:
JsName named = scope.declareName("named");
Nit: refactor string into constant.

http://codereview.chromium.org/8913006/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/backend/js/RuntimeTypeInjector.java:698:
public JsNameRef getRTTLookupMethodName(MethodElement methodElement) {
Nit: could rename to getRTTLookupMethodNameRef.

http://codereview.chromium.org/8913006/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/backend/js/RuntimeTypeInjector.java:701:
if (methodElement.getEnclosingElement().getKind().equals(ElementKind.CLASS)) {
Nit: Could use ElementKind.of(methodElement.getEnclosingElement()) ==
ElementKind.CLASS instead.

http://codereview.chromium.org/8913006/diff/1/compiler/lib/implementation/rtt.js
File compiler/lib/implementation/rtt.js (right):

http://codereview.chromium.org/8913006/diff/1/compiler/lib/implementation/rtt...
compiler/lib/implementation/rtt.js:138: var named = false;
Nit: could move below continue.

[codefu, 2011-12-15 13:01:50]

http://codereview.chromium.org/8913006/diff/1/compiler/java/com/google/dart/c...
File
compiler/java/com/google/dart/compiler/backend/js/GenerateJavascriptAST.java
(right):

http://codereview.chromium.org/8913006/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/backend/js/GenerateJavascriptAST.java:609:
JsNameRef mangledRttMethod = rtt.getRTTLookupMethodName(methodElement);
On 2011/12/14 20:48:13, mmendez wrote:
> Nit: maybe mangledRttMethodRef would be a better name?

Done.

http://codereview.chromium.org/8913006/diff/1/compiler/java/com/google/dart/c...
File compiler/java/com/google/dart/compiler/backend/js/RuntimeTypeInjector.java
(left):

http://codereview.chromium.org/8913006/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/backend/js/RuntimeTypeInjector.java:254:
} else if (hasTypeParams) {
On 2011/12/14 20:48:13, mmendez wrote:
> Nit: Alternatively, you could still only pass null only if type parameters are
> at play and named.  Otherwise, you could not emit the nulls.  Small code
> savings.

We don't know if it will be named until runtime, so the named parameter is
required. To get the savings, I can re-arrange the method so that named is the
first parameter.  Calling methods would then be required to pass null if there
isn't a name.

http://codereview.chromium.org/8913006/diff/1/compiler/java/com/google/dart/c...
File compiler/java/com/google/dart/compiler/backend/js/RuntimeTypeInjector.java
(right):

http://codereview.chromium.org/8913006/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/backend/js/RuntimeTypeInjector.java:525:
return newInvocation(newQualifiedNameRef("RTT.dynamicType.$lookupRTT"));
On 2011/12/14 20:48:13, mmendez wrote:
> Nit: not part of your patch, but you could refactor the string into a
constant.

Done.

http://codereview.chromium.org/8913006/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/backend/js/RuntimeTypeInjector.java:625:
return elementInvoke;
On 2011/12/14 20:48:13, mmendez wrote:
> Nit: could assert not null here.

Done.

http://codereview.chromium.org/8913006/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/backend/js/RuntimeTypeInjector.java:671:
JsName named = scope.declareName("named");
On 2011/12/14 20:48:13, mmendez wrote:
> Nit: refactor string into constant.

Done.

http://codereview.chromium.org/8913006/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/backend/js/RuntimeTypeInjector.java:698:
public JsNameRef getRTTLookupMethodName(MethodElement methodElement) {
On 2011/12/14 20:48:13, mmendez wrote:
> Nit: could rename to getRTTLookupMethodNameRef.

Done.

http://codereview.chromium.org/8913006/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/backend/js/RuntimeTypeInjector.java:701:
if (methodElement.getEnclosingElement().getKind().equals(ElementKind.CLASS)) {
On 2011/12/14 20:48:13, mmendez wrote:
> Nit: Could use ElementKind.of(methodElement.getEnclosingElement()) ==
> ElementKind.CLASS instead.

Done.

http://codereview.chromium.org/8913006/diff/1/compiler/lib/implementation/rtt.js
File compiler/lib/implementation/rtt.js (right):

http://codereview.chromium.org/8913006/diff/1/compiler/lib/implementation/rtt...
compiler/lib/implementation/rtt.js:138: var named = false;
On 2011/12/14 20:48:13, mmendez wrote:
> Nit: could move below continue.

Done.