Use entities in native_emitter.dart

Use entities in native_emitter.dart

R=sigmund@google.com

Committed: https://github.com/dart-lang/sdk/commit/58842470d5c1e04b62d1b81f9fc834ebd2e1d73e

[Johnni Winther, 2017-01-20 13:00:59]

johnniwinther@google.com changed reviewers:
+ sigmund@google.com

[Siggi Cherem (dart-lang), 2017-01-20 17:00:16]

lgtm

https://codereview.chromium.org/2646733006/diff/1/pkg/compiler/lib/src/js_bac...
File pkg/compiler/lib/src/js_backend/backend.dart (right):

https://codereview.chromium.org/2646733006/diff/1/pkg/compiler/lib/src/js_bac...
pkg/compiler/lib/src/js_backend/backend.dart:1638: } else if (type.isVoid) {
while we are here, we could clean up this function a bit by removing all the
`else`s. We could also avoid the duplication of some branches below if we define
something like:
   suffix = typeCast ? 'TypeCast' : 'TypeCheck';

I just played a bit with it, wdty?

  String getCheckedModeHelperNameInternal(ResolutionDartType type,
      {bool typeCast, bool nativeCheckOnly}) {
    assert(type.kind != ResolutionTypeKind.TYPEDEF);
    if (type.isMalformed) {
      // The same error is thrown for type test and type cast of a malformed
      // type so we only need one check method.
      return 'checkMalformedType';
    }

    if (type.isVoid) {
      assert(!typeCast); // Cannot cast to void.
      if (nativeCheckOnly) return null;
      return 'voidTypeCheck';
    }

    if (type.isTypeVariable) {
      return typeCast
          ? 'subtypeOfRuntimeTypeCast'
          : 'assertSubtypeOfRuntimeType';
    }

    if (type.isFunctionType) return null;

    Element element = type.element;
    bool nativeCheck =
        nativeCheckOnly || emitter.nativeEmitter.requiresNativeIsCheck(element);

    // TODO(13955), TODO(9731).  The test for non-primitive types should use an
    // interceptor.  The interceptor should be an argument to HTypeConversion so
    // that it can be optimized by standard interceptor optimizations.
    nativeCheck = true;

    var suffix = typeCast ? 'TypeCast' : 'TypeCheck';
    if (element == helpers.jsStringClass ||
        element == commonElements.stringClass) {
      if (nativeCheckOnly) return null;
      return 'string$suffix';
    }

    if (element == helpers.jsDoubleClass ||
        element == commonElements.doubleClass) {
      if (nativeCheckOnly) return null;
      return 'double$suffix';
    }

    if (element == helpers.jsNumberClass ||
        element == commonElements.numClass) {
      if (nativeCheckOnly) return null;
      return 'num$suffix';
    }

    if (element == helpers.jsBoolClass || element == commonElements.boolClass) {
      if (nativeCheckOnly) return null;
      return 'bool$suffix';
    }

    if (element == helpers.jsIntClass ||
        element == commonElements.intClass ||
        element == helpers.jsUInt32Class ||
        element == helpers.jsUInt31Class ||
        element == helpers.jsPositiveIntClass) {
      if (nativeCheckOnly) return null;
      return 'int$suffix';
    }

    if (commonElements.isNumberOrStringSupertype(element)) {
      return nativeCheck
          ? 'numberOrStringSuperNative$suffix'
          : 'numberOrStringSuper$suffix';
    }

    if (commonElements.isStringOnlySupertype(element)) {
      return nativeCheck ? 'stringSuperNative$suffix' : 'stringSuper$suffix';
    }

    if ((element == commonElements.listClass ||
            element == helpers.jsArrayClass) &&
        type.treatAsRaw) {
      if (nativeCheckOnly) return null;
      return 'list$suffix';
    }

    if (commonElements.isListSupertype(element)) {
      return nativeCheck ? 'listSuperNative$suffix' : 'listSuper$suffix';
    }

    if (type.isInterfaceType && !type.treatAsRaw) {
      return typeCast ? 'subtypeCast' : 'assertSubtype';
    }

    // TODO(karlklose): can we get rid of this branch when we use
    // interceptors?
    return nativeCheck ? 'intercepted$suffix' : 'property$suffix';
  }

https://codereview.chromium.org/2646733006/diff/1/pkg/compiler/lib/src/js_bac...
pkg/compiler/lib/src/js_backend/backend.dart:1707: if
(commonElements.isListSupertype(element)) {
if we don't remove the elses, we should at least remove the nesting here and
below in 1716

https://codereview.chromium.org/2646733006/diff/1/pkg/compiler/lib/src/js_emi...
File pkg/compiler/lib/src/js_emitter/native_emitter.dart (right):

https://codereview.chromium.org/2646733006/diff/1/pkg/compiler/lib/src/js_emi...
pkg/compiler/lib/src/js_emitter/native_emitter.dart:363: //if (element == null
|| !element.isClass) return false;
uncomment?

https://codereview.chromium.org/2646733006/diff/1/pkg/compiler/lib/src/univer...
File pkg/compiler/lib/src/universe/world_builder.dart (right):

https://codereview.chromium.org/2646733006/diff/1/pkg/compiler/lib/src/univer...
pkg/compiler/lib/src/universe/world_builder.dart:1118: /// the parameter.
I thought you were going to simply use the function-type to extract this?

https://codereview.chromium.org/2646733006/diff/1/pkg/compiler/lib/src/univer...
pkg/compiler/lib/src/universe/world_builder.dart:1120: FunctionEntity function,
void f(DartType type, String name));
will you also eventually need to mark whether the argument is required, optional
or named?

[Johnni Winther, 2017-01-23 10:00:09]

https://codereview.chromium.org/2646733006/diff/1/pkg/compiler/lib/src/js_bac...
File pkg/compiler/lib/src/js_backend/backend.dart (right):

https://codereview.chromium.org/2646733006/diff/1/pkg/compiler/lib/src/js_bac...
pkg/compiler/lib/src/js_backend/backend.dart:1638: } else if (type.isVoid) {
On 2017/01/20 17:00:15, Siggi Cherem (dart-lang) wrote:
> while we are here, we could clean up this function a bit by removing all the
> `else`s. We could also avoid the duplication of some branches below if we
define
> something like:
>    suffix = typeCast ? 'TypeCast' : 'TypeCheck';
> 
> I just played a bit with it, wdty?
> 
>   String getCheckedModeHelperNameInternal(ResolutionDartType type,
>       {bool typeCast, bool nativeCheckOnly}) {
>     assert(type.kind != ResolutionTypeKind.TYPEDEF);
>     if (type.isMalformed) {
>       // The same error is thrown for type test and type cast of a malformed
>       // type so we only need one check method.
>       return 'checkMalformedType';
>     }
> 
>     if (type.isVoid) {
>       assert(!typeCast); // Cannot cast to void.
>       if (nativeCheckOnly) return null;
>       return 'voidTypeCheck';
>     }
> 
>     if (type.isTypeVariable) {
>       return typeCast
>           ? 'subtypeOfRuntimeTypeCast'
>           : 'assertSubtypeOfRuntimeType';
>     }
> 
>     if (type.isFunctionType) return null;
> 
>     Element element = type.element;
>     bool nativeCheck =
>         nativeCheckOnly ||
emitter.nativeEmitter.requiresNativeIsCheck(element);
> 
>     // TODO(13955), TODO(9731).  The test for non-primitive types should use
an
>     // interceptor.  The interceptor should be an argument to HTypeConversion
so
>     // that it can be optimized by standard interceptor optimizations.
>     nativeCheck = true;
> 
>     var suffix = typeCast ? 'TypeCast' : 'TypeCheck';
>     if (element == helpers.jsStringClass ||
>         element == commonElements.stringClass) {
>       if (nativeCheckOnly) return null;
>       return 'string$suffix';
>     }
> 
>     if (element == helpers.jsDoubleClass ||
>         element == commonElements.doubleClass) {
>       if (nativeCheckOnly) return null;
>       return 'double$suffix';
>     }
> 
>     if (element == helpers.jsNumberClass ||
>         element == commonElements.numClass) {
>       if (nativeCheckOnly) return null;
>       return 'num$suffix';
>     }
> 
>     if (element == helpers.jsBoolClass || element == commonElements.boolClass)
{
>       if (nativeCheckOnly) return null;
>       return 'bool$suffix';
>     }
> 
>     if (element == helpers.jsIntClass ||
>         element == commonElements.intClass ||
>         element == helpers.jsUInt32Class ||
>         element == helpers.jsUInt31Class ||
>         element == helpers.jsPositiveIntClass) {
>       if (nativeCheckOnly) return null;
>       return 'int$suffix';
>     }
> 
>     if (commonElements.isNumberOrStringSupertype(element)) {
>       return nativeCheck
>           ? 'numberOrStringSuperNative$suffix'
>           : 'numberOrStringSuper$suffix';
>     }
> 
>     if (commonElements.isStringOnlySupertype(element)) {
>       return nativeCheck ? 'stringSuperNative$suffix' : 'stringSuper$suffix';
>     }
> 
>     if ((element == commonElements.listClass ||
>             element == helpers.jsArrayClass) &&
>         type.treatAsRaw) {
>       if (nativeCheckOnly) return null;
>       return 'list$suffix';
>     }
> 
>     if (commonElements.isListSupertype(element)) {
>       return nativeCheck ? 'listSuperNative$suffix' : 'listSuper$suffix';
>     }
> 
>     if (type.isInterfaceType && !type.treatAsRaw) {
>       return typeCast ? 'subtypeCast' : 'assertSubtype';
>     }
> 
>     // TODO(karlklose): can we get rid of this branch when we use
>     // interceptors?
>     return nativeCheck ? 'intercepted$suffix' : 'property$suffix';
>   }

I like it; it's much more readable. I've merge it with my change and put the
last todo into a then branch.

https://codereview.chromium.org/2646733006/diff/1/pkg/compiler/lib/src/js_bac...
pkg/compiler/lib/src/js_backend/backend.dart:1707: if
(commonElements.isListSupertype(element)) {
On 2017/01/20 17:00:15, Siggi Cherem (dart-lang) wrote:
> if we don't remove the elses, we should at least remove the nesting here and
> below in 1716

Acknowledged.

https://codereview.chromium.org/2646733006/diff/1/pkg/compiler/lib/src/js_emi...
File pkg/compiler/lib/src/js_emitter/native_emitter.dart (right):

https://codereview.chromium.org/2646733006/diff/1/pkg/compiler/lib/src/js_emi...
pkg/compiler/lib/src/js_emitter/native_emitter.dart:363: //if (element == null
|| !element.isClass) return false;
On 2017/01/20 17:00:15, Siggi Cherem (dart-lang) wrote:
> uncomment?

Removed. The test is moot.

https://codereview.chromium.org/2646733006/diff/1/pkg/compiler/lib/src/univer...
File pkg/compiler/lib/src/universe/world_builder.dart (right):

https://codereview.chromium.org/2646733006/diff/1/pkg/compiler/lib/src/univer...
pkg/compiler/lib/src/universe/world_builder.dart:1118: /// the parameter.
On 2017/01/20 17:00:15, Siggi Cherem (dart-lang) wrote:
> I thought you were going to simply use the function-type to extract this?
> 

This has to do with names of parameter (not only the types).

https://codereview.chromium.org/2646733006/diff/1/pkg/compiler/lib/src/univer...
pkg/compiler/lib/src/universe/world_builder.dart:1120: FunctionEntity function,
void f(DartType type, String name));
On 2017/01/20 17:00:15, Siggi Cherem (dart-lang) wrote:
> will you also eventually need to mark whether the argument is required,
optional
> or named?

Not sure. The JElements should not have these notions anyway.

[Johnni Winther, 2017-01-23 10:05:33]

Description was changed from

==========
Use entities in native_emitter.dart
==========

to

==========
Use entities in native_emitter.dart

R=sigmund@google.com

Committed:
https://github.com/dart-lang/sdk/commit/58842470d5c1e04b62d1b81f9fc834ebd2e1d73e
==========

[Johnni Winther, 2017-01-23 10:05:34]

Committed patchset #2 (id:20001) manually as
58842470d5c1e04b62d1b81f9fc834ebd2e1d73e (presubmit successful).

[Siggi Cherem (dart-lang), 2017-01-23 17:46:47]

https://codereview.chromium.org/2646733006/diff/1/pkg/compiler/lib/src/univer...
File pkg/compiler/lib/src/universe/world_builder.dart (right):

https://codereview.chromium.org/2646733006/diff/1/pkg/compiler/lib/src/univer...
pkg/compiler/lib/src/universe/world_builder.dart:1118: /// the parameter.
On 2017/01/23 10:00:09, Johnni Winther wrote:
> On 2017/01/20 17:00:15, Siggi Cherem (dart-lang) wrote:
> > I thought you were going to simply use the function-type to extract this?
> > 
> 
> This has to do with names of parameter (not only the types).

but I thought you only needed names for named-args, which are part of the
type-signature

[Johnni Winther, 2017-01-24 09:16:42]

https://codereview.chromium.org/2646733006/diff/1/pkg/compiler/lib/src/univer...
File pkg/compiler/lib/src/universe/world_builder.dart (right):

https://codereview.chromium.org/2646733006/diff/1/pkg/compiler/lib/src/univer...
pkg/compiler/lib/src/universe/world_builder.dart:1118: /// the parameter.
On 2017/01/23 17:46:47, Siggi Cherem (dart-lang) wrote:
> On 2017/01/23 10:00:09, Johnni Winther wrote:
> > On 2017/01/20 17:00:15, Siggi Cherem (dart-lang) wrote:
> > > I thought you were going to simply use the function-type to extract this?
> > > 
> > 
> > This has to do with names of parameter (not only the types).
> 
> but I thought you only needed names for named-args, which are part of the
> type-signature

This is called for (good) naming of parameters in JS-methods, so we could just
return `p0, p1, p2, ...`.