Avoid reparsing metadata expressions

Avoid reparsing metadata expressions

BUG= http://dartbug.com/23404
R=floitsch@google.com

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

[Siggi Cherem (dart-lang), 2015-05-06 23:03:37]

Patchset #1 (id:1) has been deleted

[Siggi Cherem (dart-lang), 2015-05-06 23:03:48]

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

[floitsch, 2015-05-07 01:14:29]

LGTM.

https://codereview.chromium.org/1126423002/diff/20001/pkg/compiler/lib/src/js...
File pkg/compiler/lib/src/js_emitter/metadata_collector.dart (right):

https://codereview.chromium.org/1126423002/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/metadata_collector.dart:74: jsAst.Expression
expression = constant == null
please keep the "(" ")" around the condition.

https://codereview.chromium.org/1126423002/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/metadata_collector.dart:124:
globalMetadata.add(expression);
easier than expected :)

https://codereview.chromium.org/1126423002/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/metadata_collector.dart:129: int
addTypeInOutputUnit(jsAst.Expression compiledType, OutputUnit outputUnit) {
I think the "compiled" refers to the fact that it is already prettyPrinted. ->
change the name.
That makes it also fit into the 80chars.

https://codereview.chromium.org/1126423002/diff/20001/pkg/compiler/lib/src/js...
File pkg/compiler/lib/src/js_emitter/new_emitter/model_emitter.dart (right):

https://codereview.chromium.org/1126423002/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/new_emitter/model_emitter.dart:424: js.Property
createGlobal(List<js.Expression> list, String global) {
It's simple enough now, that I would inline it. But feel free to keep it.

https://codereview.chromium.org/1126423002/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/new_emitter/model_emitter.dart:463: :
js.prettyPrint(new js.ArrayInitializer(types), compiler).getText());
Use `unparse`. It does the js.string and pretty-print for you.

https://codereview.chromium.org/1126423002/diff/20001/tests/compiler/dart2js_...
File tests/compiler/dart2js_extra/23404_test.dart (right):

https://codereview.chromium.org/1126423002/diff/20001/tests/compiler/dart2js_...
tests/compiler/dart2js_extra/23404_test.dart:1: foo([a="\u00a0"]) {}
Add copyright header.

Add a comment explaining what the test does.

Create a test that actually does something. Use the expect-annotations to
confuse the type-inferrer and check that the result is correct:

/// <HEADER>
import 'package:expect/expect.dart';

// Dart2js was crashing when a default argument contained an escaped character.
foo([a='\u00a0']) => a;
bar() => '';

@NoInline
@AssumeDynamic
confuse(x) => x;

main() {
  Expect.equals('\u00a0', confuse(foo)());
  Expect.equals('', confuse(bar)());
}

[Siggi Cherem (dart-lang), 2015-05-07 16:28:59]

Patchset #2 (id:40001) has been deleted

[Siggi Cherem (dart-lang), 2015-05-07 16:29:23]

thanks Florian

https://codereview.chromium.org/1126423002/diff/20001/pkg/compiler/lib/src/js...
File pkg/compiler/lib/src/js_emitter/metadata_collector.dart (right):

https://codereview.chromium.org/1126423002/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/metadata_collector.dart:74: jsAst.Expression
expression = constant == null
On 2015/05/07 01:14:29, floitsch wrote:
> please keep the "(" ")" around the condition.

Done.

https://codereview.chromium.org/1126423002/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/metadata_collector.dart:124:
globalMetadata.add(expression);
On 2015/05/07 01:14:28, floitsch wrote:
> easier than expected :)

:) totally

https://codereview.chromium.org/1126423002/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/metadata_collector.dart:129: int
addTypeInOutputUnit(jsAst.Expression compiledType, OutputUnit outputUnit) {
On 2015/05/07 01:14:29, floitsch wrote:
> I think the "compiled" refers to the fact that it is already prettyPrinted. ->
> change the name.
> That makes it also fit into the 80chars.

Done.

https://codereview.chromium.org/1126423002/diff/20001/pkg/compiler/lib/src/js...
File pkg/compiler/lib/src/js_emitter/new_emitter/model_emitter.dart (right):

https://codereview.chromium.org/1126423002/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/new_emitter/model_emitter.dart:424: js.Property
createGlobal(List<js.Expression> list, String global) {
On 2015/05/07 01:14:29, floitsch wrote:
> It's simple enough now, that I would inline it. But feel free to keep it.

Done.

https://codereview.chromium.org/1126423002/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/new_emitter/model_emitter.dart:463: :
js.prettyPrint(new js.ArrayInitializer(types), compiler).getText());
On 2015/05/07 01:14:29, floitsch wrote:
> Use `unparse`. It does the js.string and pretty-print for you.

Done.

https://codereview.chromium.org/1126423002/diff/20001/tests/compiler/dart2js_...
File tests/compiler/dart2js_extra/23404_test.dart (right):

https://codereview.chromium.org/1126423002/diff/20001/tests/compiler/dart2js_...
tests/compiler/dart2js_extra/23404_test.dart:1: foo([a="\u00a0"]) {}
On 2015/05/07 01:14:29, floitsch wrote:
> Add copyright header.
> 
> Add a comment explaining what the test does.
> 
> Create a test that actually does something. Use the expect-annotations to
> confuse the type-inferrer and check that the result is correct:
> 
> /// <HEADER>
> import 'package:expect/expect.dart';
> 
> // Dart2js was crashing when a default argument contained an escaped
character.
> foo([a='\u00a0']) => a;
> bar() => '';
> 
> @NoInline
> @AssumeDynamic
> confuse(x) => x;
> 
> main() {
>   Expect.equals('\u00a0', confuse(foo)());
>   Expect.equals('', confuse(bar)());
> }
> 

Done.

[Siggi Cherem (dart-lang), 2015-05-07 17:07:15]

Committed patchset #2 (id:60001) manually as 45600 (presubmit successful).