dart2js OldEmitter: Change to named holes in js output strings plus some extra clean-ups.

dart2js OldEmitter: Change to named holes in js output strings plus some extra clean-ups.

R=floitsch@google.com

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

[zarah, 2014-12-29 12:33:33]

Patchset #1 (id:1) has been deleted

[zarah, 2014-12-29 12:33:54]

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

[floitsch, 2014-12-29 18:09:12]

LGTM.

https://codereview.chromium.org/828753002/diff/20001/pkg/compiler/lib/src/js_...
File pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart (right):

https://codereview.chromium.org/828753002/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart:460: #allClasses =
Object.create(null);
Ok with change, but we could have just removed the comments too.
This one was really simple to read (since the order didn't even matter).

However, I would still like to inline this JS snippet, and then it makes more
sense (probably).

https://codereview.chromium.org/828753002/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart:596: jsAst.Node
optional(bool condition, jsAst.Node node) {
Since you are doing cleanups: I believe this function is unused.

https://codereview.chromium.org/828753002/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart:1333: })$N''',
{'currentScript': currentScriptAccess,
I could be wrong, but I believe that the "$N" here is completely useless: the
string is already run through the parser and then pretty-printed.

https://codereview.chromium.org/828753002/diff/20001/pkg/compiler/lib/src/js_...
File pkg/compiler/lib/src/js_emitter/old_emitter/nsm_emitter.dart (right):

https://codereview.chromium.org/828753002/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_emitter/old_emitter/nsm_emitter.dart:289: var
objectClassObject =
fits on one line?

https://codereview.chromium.org/828753002/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_emitter/old_emitter/nsm_emitter.dart:336: '   
shortNames = #diffEncoding.split(",")', {
move the "{" to the next line.
At least for the code review it's easier to see that it isn't part of the JS
code then.

https://codereview.chromium.org/828753002/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_emitter/old_emitter/nsm_emitter.dart:340:
statements.add(js.statement('var longNames = #longs.split(",")',
ok. But I didn't find it hard to read.

[zarah, 2014-12-30 10:04:10]

Committed patchset #2 (id:40001) manually as 42572 (presubmit successful).

[zarah, 2014-12-30 10:29:26]

https://codereview.chromium.org/828753002/diff/20001/pkg/compiler/lib/src/js_...
File pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart (right):

https://codereview.chromium.org/828753002/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart:460: #allClasses =
Object.create(null);
On 2014/12/29 18:09:12, floitsch wrote:
> Ok with change, but we could have just removed the comments too.
> This one was really simple to read (since the order didn't even matter).
> 
> However, I would still like to inline this JS snippet, and then it makes more
> sense (probably).

Done, in another cl.

https://codereview.chromium.org/828753002/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart:596: jsAst.Node
optional(bool condition, jsAst.Node node) {
On 2014/12/29 18:09:12, floitsch wrote:
> Since you are doing cleanups: I believe this function is unused.

Done.

https://codereview.chromium.org/828753002/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart:1333: })$N''',
{'currentScript': currentScriptAccess,
On 2014/12/29 18:09:12, floitsch wrote:
> I could be wrong, but I believe that the "$N" here is completely useless: the
> string is already run through the parser and then pretty-printed.

I agree.

https://codereview.chromium.org/828753002/diff/20001/pkg/compiler/lib/src/js_...
File pkg/compiler/lib/src/js_emitter/old_emitter/nsm_emitter.dart (right):

https://codereview.chromium.org/828753002/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_emitter/old_emitter/nsm_emitter.dart:289: var
objectClassObject =
On 2014/12/29 18:09:12, floitsch wrote:
> fits on one line?

Done.

https://codereview.chromium.org/828753002/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_emitter/old_emitter/nsm_emitter.dart:336: '   
shortNames = #diffEncoding.split(",")', {
On 2014/12/29 18:09:12, floitsch wrote:
> move the "{" to the next line.
> At least for the code review it's easier to see that it isn't part of the JS
> code then.

Done.

https://codereview.chromium.org/828753002/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_emitter/old_emitter/nsm_emitter.dart:340:
statements.add(js.statement('var longNames = #longs.split(",")',
On 2014/12/29 18:09:12, floitsch wrote:
> ok. But I didn't find it hard to read.

Acknowledged.