dart2js OldEmitter: inline FinishClasses.

dart2js OldEmitter: inline FinishClasses.

R=floitsch@google.com

Committed: https://code.google.com/p/dart/source/detail?r=42582
Reverted: https://code.google.com/p/dart/source/detail?r=42583

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

[zarah, 2014-12-23 11:17:19]

zarah@google.com changed reviewers:
+ floitsch@google.com

[floitsch, 2014-12-29 18:00:22]

LGTM.
We really need to rename the reflection_data_parser file!

https://codereview.chromium.org/819993003/diff/1/pkg/compiler/lib/src/js_emit...
File pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart (left):

https://codereview.chromium.org/819993003/diff/1/pkg/compiler/lib/src/js_emit...
pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart:347: 
nit. I would keep this new line.

https://codereview.chromium.org/819993003/diff/1/pkg/compiler/lib/src/js_emit...
File pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart (right):

https://codereview.chromium.org/819993003/diff/1/pkg/compiler/lib/src/js_emit...
pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart:24: bool hasClasses =
false;
I prefer to stay consistent: needsClassSupport ?

https://codereview.chromium.org/819993003/diff/1/pkg/compiler/lib/src/js_emit...
pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart:441: /// but should
only be run once.
specify that the "once" refers to deferred libraries.

https://codereview.chromium.org/819993003/diff/1/pkg/compiler/lib/src/js_emit...
pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart:442: jsAst.Expression
get initProcessStatics {
was already a bad name: "initX" looks/sounds like a function that does the init.
Here it is a getter, and only provides the init-code.

Since it's really small and only used in one place (which is even smaller), just
inline it at use-site.

https://codereview.chromium.org/819993003/diff/1/pkg/compiler/lib/src/js_emit...
pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart:453: (function(){
I don't see any reason why there should be the anonymous function around the
inits.
But can/should be done in a different CL.

https://codereview.chromium.org/819993003/diff/1/pkg/compiler/lib/src/js_emit...
pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart:1200:
buildLazyInitializerFunctionIfNecessary(),
just inline all three functions.
Use "if (#) ..." to optionally build code.

https://codereview.chromium.org/819993003/diff/1/pkg/compiler/lib/src/js_emit...
File pkg/compiler/lib/src/js_emitter/old_emitter/nsm_emitter.dart (right):

https://codereview.chromium.org/819993003/diff/1/pkg/compiler/lib/src/js_emit...
pkg/compiler/lib/src/js_emitter/old_emitter/nsm_emitter.dart:281:
processedClasses.collected[#],    // # is name of class Object.
long line.
The indentation was to align it with the next line. So should be easy to fix.

https://codereview.chromium.org/819993003/diff/1/pkg/compiler/lib/src/js_emit...
pkg/compiler/lib/src/js_emitter/old_emitter/nsm_emitter.dart:286: // If we are
loading a deferred library the object class will not be in
long line.

https://codereview.chromium.org/819993003/diff/1/pkg/compiler/lib/src/js_emit...
File pkg/compiler/lib/src/js_emitter/old_emitter/reflection_data_parser.dart
(right):

https://codereview.chromium.org/819993003/diff/1/pkg/compiler/lib/src/js_emit...
pkg/compiler/lib/src/js_emitter/old_emitter/reflection_data_parser.dart:237: var
isReflectable = array.length > unmangledNameIndex;
for a different CL:
inline this boolean and change the if in line 252 to:
if (#isReflectionEnabled) {
  var isReflectable = array.length > unmangledNameIndex;
  if (isReflectable) {
    ...
  }
}

https://codereview.chromium.org/819993003/diff/1/pkg/compiler/lib/src/js_emit...
pkg/compiler/lib/src/js_emitter/old_emitter/reflection_data_parser.dart:300:
processedClasses.combinedConstructorFunction =
Add comment, that the variable will be changed for every class.

https://codereview.chromium.org/819993003/diff/1/pkg/compiler/lib/src/js_emit...
pkg/compiler/lib/src/js_emitter/old_emitter/reflection_data_parser.dart:332: if
(#hasClasses) finishClasses(processedClasses);
Indentation seems off.

https://codereview.chromium.org/819993003/diff/1/pkg/compiler/lib/src/js_emit...
pkg/compiler/lib/src/js_emitter/old_emitter/reflection_data_parser.dart:376: var
constructors =
Please avoid big code moves with code changes.
It's extremely hard to see what part has just been copied and what has been
changed.

https://codereview.chromium.org/819993003/diff/1/pkg/compiler/lib/src/js_emit...
pkg/compiler/lib/src/js_emitter/old_emitter/reflection_data_parser.dart:423:
#header;
Different CL:
I wonder if we shouldn't just inline all this holes.
I don't think there is a lot of value in assigning them to a temporary variable
first. To be discussed.

[zarah, 2014-12-30 14:02:44]

https://codereview.chromium.org/819993003/diff/1/pkg/compiler/lib/src/js_emit...
File pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart (left):

https://codereview.chromium.org/819993003/diff/1/pkg/compiler/lib/src/js_emit...
pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart:347: 
On 2014/12/29 18:00:21, floitsch wrote:
> nit. I would keep this new line.

Done.

https://codereview.chromium.org/819993003/diff/1/pkg/compiler/lib/src/js_emit...
File pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart (right):

https://codereview.chromium.org/819993003/diff/1/pkg/compiler/lib/src/js_emit...
pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart:24: bool hasClasses =
false;
On 2014/12/29 18:00:22, floitsch wrote:
> I prefer to stay consistent: needsClassSupport ?

Done.

https://codereview.chromium.org/819993003/diff/1/pkg/compiler/lib/src/js_emit...
pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart:441: /// but should
only be run once.
On 2014/12/29 18:00:21, floitsch wrote:
> specify that the "once" refers to deferred libraries.

Done.

https://codereview.chromium.org/819993003/diff/1/pkg/compiler/lib/src/js_emit...
pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart:442: jsAst.Expression
get initProcessStatics {
On 2014/12/29 18:00:21, floitsch wrote:
> was already a bad name: "initX" looks/sounds like a function that does the
init.
> Here it is a getter, and only provides the init-code.
> 
> Since it's really small and only used in one place (which is even smaller),
just
> inline it at use-site.

Done.

https://codereview.chromium.org/819993003/diff/1/pkg/compiler/lib/src/js_emit...
pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart:453: (function(){
On 2014/12/29 18:00:22, floitsch wrote:
> I don't see any reason why there should be the anonymous function around the
> inits.
> But can/should be done in a different CL.

Acknowledged.

https://codereview.chromium.org/819993003/diff/1/pkg/compiler/lib/src/js_emit...
pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart:1200:
buildLazyInitializerFunctionIfNecessary(),
On 2014/12/29 18:00:21, floitsch wrote:
> just inline all three functions.
> Use "if (#) ..." to optionally build code.

Done.

https://codereview.chromium.org/819993003/diff/1/pkg/compiler/lib/src/js_emit...
File pkg/compiler/lib/src/js_emitter/old_emitter/nsm_emitter.dart (right):

https://codereview.chromium.org/819993003/diff/1/pkg/compiler/lib/src/js_emit...
pkg/compiler/lib/src/js_emitter/old_emitter/nsm_emitter.dart:281:
processedClasses.collected[#],    // # is name of class Object.
On 2014/12/29 18:00:22, floitsch wrote:
> long line.
> The indentation was to align it with the next line. So should be easy to fix.

Done.

https://codereview.chromium.org/819993003/diff/1/pkg/compiler/lib/src/js_emit...
pkg/compiler/lib/src/js_emitter/old_emitter/nsm_emitter.dart:286: // If we are
loading a deferred library the object class will not be in
On 2014/12/29 18:00:22, floitsch wrote:
> long line.

Done.

https://codereview.chromium.org/819993003/diff/1/pkg/compiler/lib/src/js_emit...
File pkg/compiler/lib/src/js_emitter/old_emitter/reflection_data_parser.dart
(right):

https://codereview.chromium.org/819993003/diff/1/pkg/compiler/lib/src/js_emit...
pkg/compiler/lib/src/js_emitter/old_emitter/reflection_data_parser.dart:237: var
isReflectable = array.length > unmangledNameIndex;
On 2014/12/29 18:00:22, floitsch wrote:
> for a different CL:
> inline this boolean and change the if in line 252 to:
> if (#isReflectionEnabled) {
>   var isReflectable = array.length > unmangledNameIndex;
>   if (isReflectable) {
>     ...
>   }
> }

Acknowledged.

https://codereview.chromium.org/819993003/diff/1/pkg/compiler/lib/src/js_emit...
pkg/compiler/lib/src/js_emitter/old_emitter/reflection_data_parser.dart:300:
processedClasses.combinedConstructorFunction =
On 2014/12/29 18:00:22, floitsch wrote:
> Add comment, that the variable will be changed for every class.

Done.

https://codereview.chromium.org/819993003/diff/1/pkg/compiler/lib/src/js_emit...
pkg/compiler/lib/src/js_emitter/old_emitter/reflection_data_parser.dart:332: if
(#hasClasses) finishClasses(processedClasses);
On 2014/12/29 18:00:22, floitsch wrote:
> Indentation seems off.

Done.

https://codereview.chromium.org/819993003/diff/1/pkg/compiler/lib/src/js_emit...
pkg/compiler/lib/src/js_emitter/old_emitter/reflection_data_parser.dart:376: var
constructors =
On 2014/12/29 18:00:22, floitsch wrote:
> Please avoid big code moves with code changes.
> It's extremely hard to see what part has just been copied and what has been
> changed.

Acknowledged.

https://codereview.chromium.org/819993003/diff/1/pkg/compiler/lib/src/js_emit...
pkg/compiler/lib/src/js_emitter/old_emitter/reflection_data_parser.dart:423:
#header;
On 2014/12/29 18:00:22, floitsch wrote:
> Different CL:
> I wonder if we shouldn't just inline all this holes.
> I don't think there is a lot of value in assigning them to a temporary
variable
> first. To be discussed.

Acknowledged.

[zarah, 2015-01-02 09:30:04]

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

[zarah, 2015-01-02 14:38:22]

Patchset #3 (id:40001) has been deleted

[zarah, 2015-01-02 14:45:05]

PTAL

[floitsch, 2015-01-02 15:20:03]

Still LGTM.

[zarah, 2015-01-05 09:15:55]

Committed patchset #4 (id:80001) manually as 42585 (presubmit successful).