dart2js OldEmitter: Small cleanups in output.

dart2js OldEmitter: Small cleanups in output.

R=floitsch@google.com

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

[zarah, 2015-01-05 14:18:14]

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

[floitsch, 2015-01-05 14:19:16]

STV.

[ahe, 2015-01-05 14:42:24]

ahe@google.com changed reviewers:
+ ahe@google.com

[ahe, 2015-01-05 14:42:25]

Big diff for "small cleanups".

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

https://codereview.chromium.org/834133002/diff/1/pkg/compiler/lib/src/js_emit...
pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart:1084: #allClasses =
Object.create(null);
Indentation.

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

https://codereview.chromium.org/834133002/diff/1/pkg/compiler/lib/src/js_emit...
pkg/compiler/lib/src/js_emitter/old_emitter/reflection_data_parser.dart:234: var
isReflectable = array.length > unmangledNameIndex;
Decoding "array" is now spread out in the code. The variable was declared here
to make it easier to read the code.

https://codereview.chromium.org/834133002/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/834133002/diff/1/pkg/compiler/lib/src/js_emit...
pkg/compiler/lib/src/js_emitter/old_emitter/reflection_data_parser.dart:251: if
(isReflectable) {
I would write:

if (!isReflectable) continue;

This way the indentation doesn't change and the code is easier to read.

Alternatively, I would remove the isReflectable variable altogether:

if (#isReflectionEnabled && isReflectable) {

[floitsch, 2015-01-05 14:52:54]

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

https://codereview.chromium.org/834133002/diff/1/pkg/compiler/lib/src/js_emit...
pkg/compiler/lib/src/js_emitter/old_emitter/reflection_data_parser.dart:234: var
isReflectable = array.length > unmangledNameIndex;
On 2015/01/05 14:42:25, ahe wrote:
> Decoding "array" is now spread out in the code. The variable was declared here
> to make it easier to read the code.

I don't think there is a good solution:
- we don't need the isReflectable when reflection isn't enabled.
- we don't want to put that line into an "if (#x)".
- we don't want to move the predicate computation away.

These are mutually exclusive. :(

I'm ok either way (although I like the 'if (#x)' the least.

https://codereview.chromium.org/834133002/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/834133002/diff/1/pkg/compiler/lib/src/js_emit...
pkg/compiler/lib/src/js_emitter/old_emitter/reflection_data_parser.dart:251: if
(isReflectable) {
On 2015/01/05 14:42:25, ahe wrote:
> I would write:
> 
> if (!isReflectable) continue;
> 
> This way the indentation doesn't change and the code is easier to read.
> 
> Alternatively, I would remove the isReflectable variable altogether:
> 
> if (#isReflectionEnabled && isReflectable) {

Adding a "continue" here assumes too much:
a- we are in a loop (not even visible in the review)
b- no code after this if is executed. Currently that's the case, but it invites
subtle bugs (only happening during reflection) for future code changes.

However: we could not intend for the "if (#xxx)" condition, since that's more of
an #ifdef anyways.

[ahe, 2015-01-05 15:05:24]

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

https://codereview.chromium.org/834133002/diff/1/pkg/compiler/lib/src/js_emit...
pkg/compiler/lib/src/js_emitter/old_emitter/reflection_data_parser.dart:234: var
isReflectable = array.length > unmangledNameIndex;
On 2015/01/05 14:52:53, floitsch wrote:
> On 2015/01/05 14:42:25, ahe wrote:
> > Decoding "array" is now spread out in the code. The variable was declared
here
> > to make it easier to read the code.
> 
> I don't think there is a good solution:
> - we don't need the isReflectable when reflection isn't enabled.
> - we don't want to put that line into an "if (#x)".

I think I know what your answer is going to be, but to be explicit about it:
why?

> - we don't want to move the predicate computation away.
> 
> These are mutually exclusive. :(
> 
> I'm ok either way (although I like the 'if (#x)' the least.

I wasn't clear: I don't see this as a big deal. I just wanted to bring attention
to why I introduced the variable in the first place.

https://codereview.chromium.org/834133002/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/834133002/diff/1/pkg/compiler/lib/src/js_emit...
pkg/compiler/lib/src/js_emitter/old_emitter/reflection_data_parser.dart:251: if
(isReflectable) {
On 2015/01/05 14:52:53, floitsch wrote:
> On 2015/01/05 14:42:25, ahe wrote:
> > I would write:
> > 
> > if (!isReflectable) continue;
> > 
> > This way the indentation doesn't change and the code is easier to read.
> > 
> > Alternatively, I would remove the isReflectable variable altogether:
> > 
> > if (#isReflectionEnabled && isReflectable) {
> 
> Adding a "continue" here assumes too much:

I don't know what I was thinking.

> a- we are in a loop (not even visible in the review)
> b- no code after this if is executed. Currently that's the case, but it
invites
> subtle bugs (only happening during reflection) for future code changes.
> 
> However: we could not intend for the "if (#xxx)" condition, since that's more
of
> an #ifdef anyways.

[floitsch, 2015-01-05 15:07:43]

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

https://codereview.chromium.org/834133002/diff/1/pkg/compiler/lib/src/js_emit...
pkg/compiler/lib/src/js_emitter/old_emitter/reflection_data_parser.dart:234: var
isReflectable = array.length > unmangledNameIndex;
On 2015/01/05 15:05:24, ahe wrote:
> On 2015/01/05 14:52:53, floitsch wrote:
> > On 2015/01/05 14:42:25, ahe wrote:
> > > Decoding "array" is now spread out in the code. The variable was declared
> here
> > > to make it easier to read the code.
> > 
> > I don't think there is a good solution:
> > - we don't need the isReflectable when reflection isn't enabled.
> > - we don't want to put that line into an "if (#x)".
> 
> I think I know what your answer is going to be, but to be explicit about it:
> why?
Mostly personal preference. I don't like "if"s all over the place. Especially if
two of them have to work together.

> 
> > - we don't want to move the predicate computation away.
> > 
> > These are mutually exclusive. :(
> > 
> > I'm ok either way (although I like the 'if (#x)' the least.
> 
> I wasn't clear: I don't see this as a big deal. I just wanted to bring
attention
> to why I introduced the variable in the first place.

[zarah, 2015-01-06 12:38:39]

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

https://codereview.chromium.org/834133002/diff/1/pkg/compiler/lib/src/js_emit...
pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart:1084: #allClasses =
Object.create(null);
On 2015/01/05 14:42:25, ahe wrote:
> Indentation.

Done.

[zarah, 2015-01-06 15:06:03]

We also use mangled names when calling Function.apply with named arguments.

[floitsch, 2015-01-06 17:10:52]

LGTM.

[zarah, 2015-01-07 08:05:22]

Committed patchset #4 (id:60001) manually as 42651 (presubmit successful).