Move class/member computation from CodegenEnqueuer to CodegenWorldBuilder.

Move class/member computation from CodegenEnqueuer to CodegenWorldBuilder.

R=sigmund@google.com

Committed: https://github.com/dart-lang/sdk/commit/4522e3a80619de3e9b9dec0ceb8f67bca27084da

[Johnni Winther, 2016-12-19 15:37:28]

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

[Johnni Winther, 2016-12-20 08:36:24]

ping

[Siggi Cherem (dart-lang), 2016-12-20 19:12:33]

lgtm

https://codereview.chromium.org/2587913004/diff/20001/pkg/compiler/lib/src/un...
File pkg/compiler/lib/src/universe/world_builder.dart (right):

https://codereview.chromium.org/2587913004/diff/20001/pkg/compiler/lib/src/un...
pkg/compiler/lib/src/universe/world_builder.dart:1059: classUsed(usage.cls,
usage.implement());
is `usage.cls` expected to be the same as `cls`?

if, so, consider rewriting below as:

classUsed(cls, _getClassUsage(cls).implement());
cls.allSupertypes
    .map((s) => s.element)
    .forEach((ClassElement supertype) {
  if (_implementedClasses.add(supertype)) {
    classUsed(supertype, _getClassUsage(supertype).implement());
  }
});

https://codereview.chromium.org/2587913004/diff/20001/pkg/compiler/lib/src/un...
pkg/compiler/lib/src/universe/world_builder.dart:1106: if
(dynamicUse.appliesUnnamed(member.entity, _world)) {
I know this is similar to what we had before, so feel free to do later/separate
CL.

It seems that we can do some minor simplifications to reduce code duplication
here with the calls to _processInstanceMembers and _processInstanceFunctions.
For example, we could move most of the code here, except `member.invoke()` into
the helper methods.

One idea could be to do:

   typedef EnumSet<MemberUse> MemberUsageAction(_MemberUsage mu);

  _processInstanceMember(String methodName, Map<...> invokedSelectors,
MemberUsageAction action) {
      if (_registerNewSelector(dynamicUse, invokedSelectors)) {
        _processDynamicUseHelper(_instanceMembersByName, memberName, action);
        return true;
      }
      return false;
  }

  _processDynamicUseHelper(Map map, String methodName, MemberUsageAction action)
{
     _processSet(map, methodName, (_MemberUsage member) {        
        if (dynamicUse.appliesUnnamed(member.entity, _world)) {
           memberUsed(member.entity, action(member));
           return true;
        }
        return false;
     });
  }


Then this switch becomes:

  case DynamicUseKind.INVOKE:
      return _processInstanceMember(methodName, 
          invokedNames, (m) => m.invoke());

  case DynamicUseKind.GET:
      var res = _processInstanceMember(
          methodName, invokedGetters, (m) => m.read());
      res = _processInstanceFunction(methodName, 
          invokedGetters,(m) => m.read()) || res;
      return res;

  case DynamicUseKind.SET:
     return _processInstanceMember(methodName,
          invokedSetters, (m) => m.write());

[Johnni Winther, 2016-12-21 09:19:08]

https://codereview.chromium.org/2587913004/diff/20001/pkg/compiler/lib/src/un...
File pkg/compiler/lib/src/universe/world_builder.dart (right):

https://codereview.chromium.org/2587913004/diff/20001/pkg/compiler/lib/src/un...
pkg/compiler/lib/src/universe/world_builder.dart:1059: classUsed(usage.cls,
usage.implement());
On 2016/12/20 19:12:33, Siggi Cherem (dart-lang) wrote:
> is `usage.cls` expected to be the same as `cls`?
> 
> if, so, consider rewriting below as:
> 
> classUsed(cls, _getClassUsage(cls).implement());
> cls.allSupertypes
>     .map((s) => s.element)
>     .forEach((ClassElement supertype) {
>   if (_implementedClasses.add(supertype)) {
>     classUsed(supertype, _getClassUsage(supertype).implement());
>   }
> });

Done.

https://codereview.chromium.org/2587913004/diff/20001/pkg/compiler/lib/src/un...
pkg/compiler/lib/src/universe/world_builder.dart:1106: if
(dynamicUse.appliesUnnamed(member.entity, _world)) {
On 2016/12/20 19:12:33, Siggi Cherem (dart-lang) wrote:
> I know this is similar to what we had before, so feel free to do
later/separate
> CL.
> 
> It seems that we can do some minor simplifications to reduce code duplication
> here with the calls to _processInstanceMembers and _processInstanceFunctions.
> For example, we could move most of the code here, except `member.invoke()`
into
> the helper methods.
> 
> One idea could be to do:
> 
>    typedef EnumSet<MemberUse> MemberUsageAction(_MemberUsage mu);
> 
>   _processInstanceMember(String methodName, Map<...> invokedSelectors,
> MemberUsageAction action) {
>       if (_registerNewSelector(dynamicUse, invokedSelectors)) {
>         _processDynamicUseHelper(_instanceMembersByName, memberName, action);
>         return true;
>       }
>       return false;
>   }
> 
>   _processDynamicUseHelper(Map map, String methodName, MemberUsageAction
action)
> {
>      _processSet(map, methodName, (_MemberUsage member) {        
>         if (dynamicUse.appliesUnnamed(member.entity, _world)) {
>            memberUsed(member.entity, action(member));
>            return true;
>         }
>         return false;
>      });
>   }
> 
> 
> Then this switch becomes:
> 
>   case DynamicUseKind.INVOKE:
>       return _processInstanceMember(methodName, 
>           invokedNames, (m) => m.invoke());
> 
>   case DynamicUseKind.GET:
>       var res = _processInstanceMember(
>           methodName, invokedGetters, (m) => m.read());
>       res = _processInstanceFunction(methodName, 
>           invokedGetters,(m) => m.read()) || res;
>       return res;
> 
>   case DynamicUseKind.SET:
>      return _processInstanceMember(methodName,
>           invokedSetters, (m) => m.write());
> 
> 

Good idea! Will do a later CL and in ResolutionWorldBuilderImpl as well.

[Johnni Winther, 2016-12-21 09:43:17]

Description was changed from

==========
Move class/member computation from CodegenEnqueuer to CodegenWorldBuilder.
==========

to

==========
Move class/member computation from CodegenEnqueuer to CodegenWorldBuilder.

R=sigmund@google.com

Committed:
https://github.com/dart-lang/sdk/commit/4522e3a80619de3e9b9dec0ceb8f67bca27084da
==========

[Johnni Winther, 2016-12-21 09:43:20]

Committed patchset #3 (id:40001) manually as
4522e3a80619de3e9b9dec0ceb8f67bca27084da (presubmit successful).