Add runtimeType() to Object which returns canonicalized instances of Type.

Add runtimeType() to Object which returns canonicalized instances of Type.

BUG=3903
Committed: https://code.google.com/p/dart/source/detail?r=12491

[kasperl, 2012-09-14 07:26:48]

LGTM, but I think the code in the builder could use some refactoring.

https://chromiumcodereview.appspot.com/10908142/diff/1021/lib/compiler/implem...
File lib/compiler/implementation/lib/js_helper.dart (right):

https://chromiumcodereview.appspot.com/10908142/diff/1021/lib/compiler/implem...
lib/compiler/implementation/lib/js_helper.dart:1294: var s;
This s will be visible to the outside. Give it a better name?

https://chromiumcodereview.appspot.com/10908142/diff/1021/lib/compiler/implem...
File lib/compiler/implementation/ssa/builder.dart (right):

https://chromiumcodereview.appspot.com/10908142/diff/1021/lib/compiler/implem...
lib/compiler/implementation/ssa/builder.dart:2666: String runtimeTypeString =
"'${element.name.slowToString()}";
Move the computation of the runtimeTypeString into its own method. Consider
using a string buffer for building it instead and then do something like:

   StringBuffer buffer = new StringBuffer();
   ...
   return "'$buffer'";

to make it clear that the single quotes match up. Maybe you can even use the
same method for computing the typeVariablesString if you use a closure for
computing what to add to the buffer?

https://chromiumcodereview.appspot.com/10908142/diff/1021/lib/compiler/implem...
lib/compiler/implementation/ssa/builder.dart:2695: String runtimeCode = '{';
Use StringBuffer.

https://chromiumcodereview.appspot.com/10908142/diff/1021/lib/core/object.dart
File lib/core/object.dart (right):

https://chromiumcodereview.appspot.com/10908142/diff/1021/lib/core/object.dar...
lib/core/object.dart:5: class Type {}
Move this below Object. I guess Type should be abstract?

[ngeoffray, 2012-09-14 07:30:21]

https://chromiumcodereview.appspot.com/10908142/diff/1021/lib/compiler/implem...
File lib/compiler/implementation/enqueue.dart (right):

https://chromiumcodereview.appspot.com/10908142/diff/1021/lib/compiler/implem...
lib/compiler/implementation/enqueue.dart:252: compiler.enabledRuntimeType =
true;
I'd prefer doing the check on the Element. There are usually way more selectors
than elements.

https://chromiumcodereview.appspot.com/10908142/diff/1021/lib/compiler/implem...
File lib/compiler/implementation/lib/js_helper.dart (right):

https://chromiumcodereview.appspot.com/10908142/diff/1021/lib/compiler/implem...
lib/compiler/implementation/lib/js_helper.dart:1294: var s;
var s -> String s?

https://chromiumcodereview.appspot.com/10908142/diff/1021/lib/compiler/implem...
File lib/compiler/implementation/ssa/builder.dart (right):

https://chromiumcodereview.appspot.com/10908142/diff/1021/lib/compiler/implem...
lib/compiler/implementation/ssa/builder.dart:2660: HInstruction newObject) {
I believe you could move the creation of the strings out of the builder right?
Like in RuntimeTypeInformation? Then the builder would just need to create the
HForeign and the logic for rti would be elsewhere.

[karlklose, 2012-09-14 12:07:13]

PTAL.

https://codereview.chromium.org/10908142/diff/1021/lib/compiler/implementatio...
File lib/compiler/implementation/enqueue.dart (right):

https://codereview.chromium.org/10908142/diff/1021/lib/compiler/implementatio...
lib/compiler/implementation/enqueue.dart:252: compiler.enabledRuntimeType =
true;
On 2012/09/14 07:30:21, ngeoffray wrote:
> I'd prefer doing the check on the Element. There are usually way more
selectors
> than elements.

Done.

https://codereview.chromium.org/10908142/diff/1021/lib/compiler/implementatio...
File lib/compiler/implementation/lib/js_helper.dart (right):

https://codereview.chromium.org/10908142/diff/1021/lib/compiler/implementatio...
lib/compiler/implementation/lib/js_helper.dart:1294: var s;
On 2012/09/14 07:26:48, kasperl wrote:
> This s will be visible to the outside. Give it a better name?

Done.

https://codereview.chromium.org/10908142/diff/1021/lib/compiler/implementatio...
lib/compiler/implementation/lib/js_helper.dart:1294: var s;
On 2012/09/14 07:30:21, ngeoffray wrote:
> var s -> String s?

Done.

https://codereview.chromium.org/10908142/diff/1021/lib/core/object.dart
File lib/core/object.dart (right):

https://codereview.chromium.org/10908142/diff/1021/lib/core/object.dart#newcode5
lib/core/object.dart:5: class Type {}
On 2012/09/14 07:26:48, kasperl wrote:
> Move this below Object. I guess Type should be abstract?

Done.

[kasperl, 2012-09-14 12:35:37]

https://codereview.chromium.org/10908142/diff/12011/lib/compiler/implementati...
File lib/compiler/implementation/ssa/builder.dart (right):

https://codereview.chromium.org/10908142/diff/12011/lib/compiler/implementati...
lib/compiler/implementation/ssa/builder.dart:2666: HInstruction typeInfo;
Isn't this typeInfo variable unused (shadowed)?

https://codereview.chromium.org/10908142/diff/12011/lib/compiler/implementati...
lib/compiler/implementation/ssa/builder.dart:2673: runtimeType =
Can't you declare runtimeType here?

https://codereview.chromium.org/10908142/diff/12011/lib/compiler/implementati...
lib/compiler/implementation/ssa/builder.dart:2686: HInstruction typeInfo =
Add a helper for this creation of a new foreign string thing? Seems like it
could be used in three cases (if you parameterize it with code, type, and
inputs). Maybe it would actually be useful in other places too?

https://codereview.chromium.org/10908142/diff/12011/lib/compiler/implementati...
File lib/compiler/implementation/ssa/ssa.dart (right):

https://codereview.chromium.org/10908142/diff/12011/lib/compiler/implementati...
lib/compiler/implementation/ssa/ssa.dart:36: if (type is InterfaceType) {
Can this be a method call on type?

https://codereview.chromium.org/10908142/diff/12011/lib/compiler/implementati...
lib/compiler/implementation/ssa/ssa.dart:43: static void foreach(Link
collection,
forEach?

https://codereview.chromium.org/10908142/diff/12011/lib/compiler/implementati...
lib/compiler/implementation/ssa/ssa.dart:68: buffer.add(isFirst ? "<$value" : ",
$value");
If you remove the < here and move it out so you do something like this:

   String elementName = element.name.slowToString();
   if (element.typeVariables.isEmpty()) return elementName;
   StringBuffer buffer = new StringBuffer();
   forEach(.., (..) {
     
   });
   return "'$elementName<$buffer>'";

That also opens up for dealing with the isFirst thing in a consistent way. Maybe
forEach could be changed to do something like this:

  static String forEach(
       Link collection,
       int numberOfInputs,
       String stringify(TypeVariableType variable, bool hasValue)) {
     int currentVariable = 0;
     bool isFirst = true;
     StringBuffer buffer = new StringBuffer();
     collection.forEach((TypeVariableType variable) {
       if (!isFirst) buffer.add(", ");
       bool hasValue = currentVariable < numberOfInputs;
       buffer.add(stringify(variable, hasValue));
       isFirst = false;
       currentVariable++;
     });
     return buffer.toString();
   }

which would simplify generateRuntimeTypeString to look something like this:

  static generateRuntimeTypeString(...) {
    String elementName = element.name.slowToString();
    if (element.typeVariables.isEmpty()) return elementName;
    String stringify(_, hasValue) 
        => hasValue ? "' + # + '" : "Dynamic";
    String arguments = forEach(element.typeVariables,
                               numberOfInputs,
                               stringify);
    return "'$elementName<$arguments>'";
  }

Makes sense? You may want to change the name of forEach if you go this route.

https://codereview.chromium.org/10908142/diff/12011/lib/compiler/implementati...
lib/compiler/implementation/ssa/ssa.dart:84: int currentVariable = 0;
Unused?

[karlklose, 2012-09-17 14:50:37]

https://codereview.chromium.org/10908142/diff/12011/lib/compiler/implementati...
File lib/compiler/implementation/ssa/builder.dart (right):

https://codereview.chromium.org/10908142/diff/12011/lib/compiler/implementati...
lib/compiler/implementation/ssa/builder.dart:2666: HInstruction typeInfo;
Done, removed.

https://codereview.chromium.org/10908142/diff/12011/lib/compiler/implementati...
lib/compiler/implementation/ssa/builder.dart:2673: runtimeType =
On 2012/09/14 12:35:37, kasperl wrote:
> Can't you declare runtimeType here?

Done.

https://codereview.chromium.org/10908142/diff/12011/lib/compiler/implementati...
lib/compiler/implementation/ssa/builder.dart:2686: HInstruction typeInfo =
On 2012/09/14 12:35:37, kasperl wrote:
> Add a helper for this creation of a new foreign string thing? Seems like it
> could be used in three cases (if you parameterize it with code, type, and
> inputs). Maybe it would actually be useful in other places too?

Done.

https://codereview.chromium.org/10908142/diff/12011/lib/compiler/implementati...
File lib/compiler/implementation/ssa/ssa.dart (right):

https://codereview.chromium.org/10908142/diff/12011/lib/compiler/implementati...
lib/compiler/implementation/ssa/ssa.dart:43: static void foreach(Link
collection,
Changed the name.

https://codereview.chromium.org/10908142/diff/12011/lib/compiler/implementati...
lib/compiler/implementation/ssa/ssa.dart:68: buffer.add(isFirst ? "<$value" : ",
$value");
On 2012/09/14 12:35:37, kasperl wrote:
> If you remove the < here and move it out so you do something like this:
> 
>    String elementName = element.name.slowToString();
>    if (element.typeVariables.isEmpty()) return elementName;
>    StringBuffer buffer = new StringBuffer();
>    forEach(.., (..) {
>      
>    });
>    return "'$elementName<$buffer>'";
> 
> That also opens up for dealing with the isFirst thing in a consistent way.
Maybe
> forEach could be changed to do something like this:
> 
>   static String forEach(
>        Link collection,
>        int numberOfInputs,
>        String stringify(TypeVariableType variable, bool hasValue)) {
>      int currentVariable = 0;
>      bool isFirst = true;
>      StringBuffer buffer = new StringBuffer();
>      collection.forEach((TypeVariableType variable) {
>        if (!isFirst) buffer.add(", ");
>        bool hasValue = currentVariable < numberOfInputs;
>        buffer.add(stringify(variable, hasValue));
>        isFirst = false;
>        currentVariable++;
>      });
>      return buffer.toString();
>    }
> 
> which would simplify generateRuntimeTypeString to look something like this:
> 
>   static generateRuntimeTypeString(...) {
>     String elementName = element.name.slowToString();
>     if (element.typeVariables.isEmpty()) return elementName;
>     String stringify(_, hasValue) 
>         => hasValue ? "' + # + '" : "Dynamic";
>     String arguments = forEach(element.typeVariables,
>                                numberOfInputs,
>                                stringify);
>     return "'$elementName<$arguments>'";
>   }
> 
> Makes sense? You may want to change the name of forEach if you go this route.

Done.

https://codereview.chromium.org/10908142/diff/12011/lib/compiler/implementati...
lib/compiler/implementation/ssa/ssa.dart:84: int currentVariable = 0;
On 2012/09/14 12:35:37, kasperl wrote:
> Unused?

Done.

[ngeoffray, 2012-09-17 15:31:14]

LGTM

https://codereview.chromium.org/10908142/diff/4003/lib/compiler/implementatio...
File lib/compiler/implementation/lib/js_helper.dart (right):

https://codereview.chromium.org/10908142/diff/4003/lib/compiler/implementatio...
lib/compiler/implementation/lib/js_helper.dart:1335: Type runtimeType =
JS('Object', @'$.runtimeTypeCache[#]', key);
'Object' -> 'Type'

https://codereview.chromium.org/10908142/diff/4003/lib/compiler/implementatio...
lib/compiler/implementation/lib/js_helper.dart:1335: Type runtimeType =
JS('Object', @'$.runtimeTypeCache[#]', key);
Please use JS_CURRENT_ISOLATE() instead of $.

https://codereview.chromium.org/10908142/diff/4003/lib/compiler/implementatio...
lib/compiler/implementation/lib/js_helper.dart:1338: JS('void',
@'$.runtimeTypeCache[#] = #', key, runtimeType);
ditto

https://codereview.chromium.org/10908142/diff/4003/lib/compiler/implementatio...
File lib/compiler/implementation/ssa/ssa.dart (right):

https://codereview.chromium.org/10908142/diff/4003/lib/compiler/implementatio...
lib/compiler/implementation/ssa/ssa.dart:43: static String
forEachTypeVariable(Link collection,
Add a top-level comment on what this method does.

https://codereview.chromium.org/10908142/diff/4003/lib/compiler/implementatio...
lib/compiler/implementation/ssa/ssa.dart:43: static String
forEachTypeVariable(Link collection,
Link collection -> Link<TypeVariableType> collection

https://codereview.chromium.org/10908142/diff/4003/lib/compiler/implementatio...
lib/compiler/implementation/ssa/ssa.dart:70: String stringify(_, hasValue) {
Please add types to the parameters.

https://codereview.chromium.org/10908142/diff/4003/lib/compiler/implementatio...
lib/compiler/implementation/ssa/ssa.dart:71: return hasValue ? "' + # + '" :
"Dynamic";
Use the => notation?

https://codereview.chromium.org/10908142/diff/4003/lib/compiler/implementatio...
lib/compiler/implementation/ssa/ssa.dart:71: return hasValue ? "' + # + '" :
"Dynamic";
Why no ' + "Dynamic" + ' for the second case?

https://codereview.chromium.org/10908142/diff/4003/lib/compiler/implementatio...
lib/compiler/implementation/ssa/ssa.dart:87: StringBuffer buffer = new
StringBuffer();
This is unused.

https://codereview.chromium.org/10908142/diff/4003/lib/compiler/implementatio...
lib/compiler/implementation/ssa/ssa.dart:89: (variable, bool hasValue) {
I find it easier to have this closure as a function statement (like in line 71)

https://codereview.chromium.org/10908142/diff/4003/lib/compiler/implementatio...
File lib/compiler/implementation/world.dart (right):

https://codereview.chromium.org/10908142/diff/4003/lib/compiler/implementatio...
lib/compiler/implementation/world.dart:76: return
classesNeedingRti.contains(cls) || compiler.enabledRuntimeType;
Ouch. Is it there that you should also check which types are instantiated?
Please add a TODO.

Also, do you plan on cutting the number of classes needing rti by using the type
information at the ssa level of the receiver of dynamicType?

[kasperl, 2012-09-18 06:06:11]

LGTM if you address Nicolas' comments.

https://chromiumcodereview.appspot.com/10908142/diff/4003/lib/compiler/implem...
File lib/compiler/implementation/ssa/ssa.dart (right):

https://chromiumcodereview.appspot.com/10908142/diff/4003/lib/compiler/implem...
lib/compiler/implementation/ssa/ssa.dart:43: static String
forEachTypeVariable(Link collection,
Maybe change the name of this to stringifyTypeVariables?

[karlklose, 2012-09-19 06:40:45]

https://chromiumcodereview.appspot.com/10908142/diff/4003/lib/compiler/implem...
File lib/compiler/implementation/lib/js_helper.dart (right):

https://chromiumcodereview.appspot.com/10908142/diff/4003/lib/compiler/implem...
lib/compiler/implementation/lib/js_helper.dart:1335: Type runtimeType =
JS('Object', @'$.runtimeTypeCache[#]', key);
On 2012/09/17 15:31:14, ngeoffray wrote:
> 'Object' -> 'Type'

Done.

https://chromiumcodereview.appspot.com/10908142/diff/4003/lib/compiler/implem...
lib/compiler/implementation/lib/js_helper.dart:1335: Type runtimeType =
JS('Object', @'$.runtimeTypeCache[#]', key);
On 2012/09/17 15:31:14, ngeoffray wrote:
> Please use JS_CURRENT_ISOLATE() instead of $.

Done.

https://chromiumcodereview.appspot.com/10908142/diff/4003/lib/compiler/implem...
lib/compiler/implementation/lib/js_helper.dart:1338: JS('void',
@'$.runtimeTypeCache[#] = #', key, runtimeType);
On 2012/09/17 15:31:14, ngeoffray wrote:
> ditto

Done.

https://chromiumcodereview.appspot.com/10908142/diff/4003/lib/compiler/implem...
File lib/compiler/implementation/ssa/ssa.dart (right):

https://chromiumcodereview.appspot.com/10908142/diff/4003/lib/compiler/implem...
lib/compiler/implementation/ssa/ssa.dart:43: static String
forEachTypeVariable(Link collection,
On 2012/09/17 15:31:14, ngeoffray wrote:
> Add a top-level comment on what this method does.

Done.

https://chromiumcodereview.appspot.com/10908142/diff/4003/lib/compiler/implem...
lib/compiler/implementation/ssa/ssa.dart:70: String stringify(_, hasValue) {
On 2012/09/17 15:31:14, ngeoffray wrote:
> Please add types to the parameters.

Done.

https://chromiumcodereview.appspot.com/10908142/diff/4003/lib/compiler/implem...
lib/compiler/implementation/ssa/ssa.dart:70: String stringify(_, hasValue) {
I added it for the hasValue. I use '_' as a marker that I am not interested in
the argument and I do not think that adding a type would increase readability
here.

https://chromiumcodereview.appspot.com/10908142/diff/4003/lib/compiler/implem...
lib/compiler/implementation/ssa/ssa.dart:71: return hasValue ? "' + # + '" :
"Dynamic";
On 2012/09/17 15:31:14, ngeoffray wrote:
> Use the => notation?

Done.

https://chromiumcodereview.appspot.com/10908142/diff/4003/lib/compiler/implem...
lib/compiler/implementation/ssa/ssa.dart:71: return hasValue ? "' + # + '" :
"Dynamic";
On 2012/09/17 15:31:14, ngeoffray wrote:
> Use the => notation?

Done.

https://chromiumcodereview.appspot.com/10908142/diff/4003/lib/compiler/implem...
lib/compiler/implementation/ssa/ssa.dart:71: return hasValue ? "' + # + '" :
"Dynamic";
Which case do you mean?

https://chromiumcodereview.appspot.com/10908142/diff/4003/lib/compiler/implem...
lib/compiler/implementation/ssa/ssa.dart:87: StringBuffer buffer = new
StringBuffer();
Done, removed.

https://chromiumcodereview.appspot.com/10908142/diff/4003/lib/compiler/implem...
lib/compiler/implementation/ssa/ssa.dart:89: (variable, bool hasValue) {
On 2012/09/17 15:31:14, ngeoffray wrote:
> I find it easier to have this closure as a function statement (like in line
71)

Done.

https://chromiumcodereview.appspot.com/10908142/diff/4003/lib/compiler/implem...
File lib/compiler/implementation/world.dart (right):

https://chromiumcodereview.appspot.com/10908142/diff/4003/lib/compiler/implem...
lib/compiler/implementation/world.dart:76: return
classesNeedingRti.contains(cls) || compiler.enabledRuntimeType;
No, I don't think we need to check that here. Currently we simply need the type
arguments to construct the runtime type string representation.