dart2js: Use frequency of occurence to sort metadata indices.

dart2js: Use frequency of occurence to sort metadata indices.

BUG=
R=johnniwinther@google.com, karlklose@google.com, sigurdm@google.com, sra@google.com

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

[herhut, 2015-05-27 13:58:27]

herhut@google.com changed reviewers:
+ johnniwinther@google.com, karlklose@google.com, sigurdm@google.com,
sra@google.com

[herhut, 2015-05-27 14:09:34]

Patchset #1 (id:1) has been deleted

[herhut, 2015-05-27 14:12:27]

Johnni, PTAL the new constants, in particular the DummyConstantExpression

sra: PTAL the changes to js_ast

sigurdm: PTAL the changes to the async rewriter. Those should be trivial

Karl: PTAL the rest :)

[sigurdm, 2015-05-27 14:26:41]

async-rewrite: LGTM

https://codereview.chromium.org/1153243003/diff/30001/pkg/compiler/lib/src/js...
File pkg/compiler/lib/src/js_backend/backend.dart (right):

https://codereview.chromium.org/1153243003/diff/30001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_backend/backend.dart:218: static const String
typeVariableReference = "TYPEVARIALE_REFERENCE";
DBC: Typo in string

[sra1, 2015-05-27 19:38:51]

https://codereview.chromium.org/1153243003/diff/30001/pkg/compiler/lib/src/co...
File pkg/compiler/lib/src/constants/values.dart (right):

https://codereview.chromium.org/1153243003/diff/30001/pkg/compiler/lib/src/co...
pkg/compiler/lib/src/constants/values.dart:607: final payload;
TL;DR: should this class be split into 2 to 3 classes?

The original dummy constant was only copied (e.g. passed as an argument to an
unused parameter).  It did not matter at all what the textual JS representation
was.

If there is a different kind of 'constant' that actually flows to a nontrivial
use I think it should be a different kind of ConstantValue.  When 'payload' is
an expression, what how is that expression used at runtime?

I see that some uses of payload are js.Expression and some are TypeMask.  Are
there other uses?

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

https://codereview.chromium.org/1153243003/diff/30001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/metadata_collector.dart:65: return -336699;
Should this be
  assert(isBound);
?

https://codereview.chromium.org/1153243003/diff/30001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/metadata_collector.dart:211: Placeholder
getMetadataPlaceholder([debug]) => new _ForwardingMetadataEntry(debug);
line length

https://codereview.chromium.org/1153243003/diff/30001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/metadata_collector.dart:294:
jsAst.ArrayInitializer finalizeMap(Map<dynamic, _BoundMetadataEntry> map) {
Indentation

https://codereview.chromium.org/1153243003/diff/30001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/metadata_collector.dart:297: entries.sort();
Sort is not stable.
Equivalence classes by reference count will be ordered arbitrarily.

https://codereview.chromium.org/1153243003/diff/30001/pkg/compiler/lib/src/js...
File pkg/compiler/lib/src/js_emitter/model.dart (right):

https://codereview.chromium.org/1153243003/diff/30001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/model.dart:238: final js.Expression
functionTypeIndex;
Could this be js.TokenNumber ?

https://codereview.chromium.org/1153243003/diff/30001/pkg/compiler/lib/src/js...
File pkg/compiler/lib/src/js_emitter/runtime_type_generator.dart (right):

https://codereview.chromium.org/1153243003/diff/30001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/runtime_type_generator.dart:15: jsAst.Expression
functionTypeIndex;
Can this be a stronger type?

https://codereview.chromium.org/1153243003/diff/30001/pkg/js_ast/lib/src/node...
File pkg/js_ast/lib/src/nodes.dart (right):

https://codereview.chromium.org/1153243003/diff/30001/pkg/js_ast/lib/src/node...
pkg/js_ast/lib/src/nodes.dart:933: abstract class Token extends Literal {
These are abstract classes that are overridden.
Describe how they should be used.

I think 'DeferredToken' and 'DeferredNumberToken' etc would be a better names. 
'Late' would be an acceptable synonym for 'Deferred'

https://codereview.chromium.org/1153243003/diff/30001/pkg/js_ast/lib/src/node...
pkg/js_ast/lib/src/nodes.dart:951: abstract class TokenExpression extends Token
{
What is a TokenExpression?
DeferredExpression?

https://codereview.chromium.org/1153243003/diff/30001/pkg/js_ast/lib/src/node...
pkg/js_ast/lib/src/nodes.dart:954: Expression get value;
What is the precedence?
PRIMARY is almost certainly wrong.

https://codereview.chromium.org/1153243003/diff/30001/pkg/js_ast/lib/src/prin...
File pkg/js_ast/lib/src/printer.dart (right):

https://codereview.chromium.org/1153243003/diff/30001/pkg/js_ast/lib/src/prin...
pkg/js_ast/lib/src/printer.dart:957: node.value.accept(this);
Either the precedence needs to be fixed or this code needs to account for
'incorrect' precedence.

https://codereview.chromium.org/1153243003/diff/30001/pkg/js_ast/lib/src/temp...
File pkg/js_ast/lib/src/template.dart (right):

https://codereview.chromium.org/1153243003/diff/30001/pkg/js_ast/lib/src/temp...
pkg/js_ast/lib/src/template.dart:657: Instantiator
visitTokenExpression(TokenExpression node) =>
=> same;

https://codereview.chromium.org/1153243003/diff/30001/pkg/js_ast/lib/src/temp...
pkg/js_ast/lib/src/template.dart:660: Instantiator visitTokenNumber(TokenNumber
node) =>
ditto

[sra1, 2015-05-27 20:30:16]

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

https://codereview.chromium.org/1153243003/diff/30001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/metadata_collector.dart:302: }
Longer term I think we need to be more sophisticated to control build-to-build
differences.  We could bucket entries by number of digits and then allocate
within the bucket by some deterministic feature-based allocation.  This would
protect the large number of low reference-count entries being moved due to
something being 'promoted' over them by an additional reference or two.

https://codereview.chromium.org/1153243003/diff/30001/pkg/js_ast/lib/src/buil...
File pkg/js_ast/lib/src/builder.dart (right):

https://codereview.chromium.org/1153243003/diff/30001/pkg/js_ast/lib/src/buil...
pkg/js_ast/lib/src/builder.dart:341: _parts = <Literal>[stringPart('"')]
This stringPart can be shared between begining and end, or over all
concatenateStrings.

https://codereview.chromium.org/1153243003/diff/30001/pkg/js_ast/lib/src/prin...
File pkg/js_ast/lib/src/printer.dart (right):

https://codereview.chromium.org/1153243003/diff/30001/pkg/js_ast/lib/src/prin...
pkg/js_ast/lib/src/printer.dart:965: out(" ");
I'd like to see a test that covers this case.

[sra1, 2015-05-27 20:31:18]

I'm excited to see some results but I can't patch.
Do you know the github hash to patch against?

[karlklose, 2015-05-28 09:39:53]

LGTM with comments.

I wonder what the general strategy for handling inconsistencies in the metadata
mapping is: sometimes you seem to emit bogus code, but I am not sure how it is
handled in the rest of the emitter. Should we always fail early with an internal
error or at least an assertion?

https://codereview.chromium.org/1153243003/diff/30001/pkg/compiler/lib/src/js...
File pkg/compiler/lib/src/js_backend/backend.dart (right):

https://codereview.chromium.org/1153243003/diff/30001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_backend/backend.dart:215: class DummyConstantKinds {
I think it use sites of these constants read better if you called the class
'DummyConstantKind' and wrote the constants in all caps.

https://codereview.chromium.org/1153243003/diff/30001/pkg/compiler/lib/src/js...
File pkg/compiler/lib/src/js_emitter/helpers.dart (right):

https://codereview.chromium.org/1153243003/diff/30001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/helpers.dart:22: void setHash(String hash) {
Maybe this pattern could be extraced in a class 'Lazy<T>'? The output unit seems
to be here for debugging purposes only.

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

https://codereview.chromium.org/1153243003/diff/30001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/metadata_collector.dart:10: int get _rc;
Could you add comments for this class and _rc?

https://codereview.chromium.org/1153243003/diff/30001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/metadata_collector.dart:30: if (!isFinalized)
print ("OHA! I have no value!");
Remove/replace debug print.

https://codereview.chromium.org/1153243003/diff/30001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/metadata_collector.dart:60: return new
jsAst.LiteralString('"BOGUS: $debugData"');
assert(isBound) or throw?

https://codereview.chromium.org/1153243003/diff/30001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/metadata_collector.dart:280: if (entry._rc == 0)
{
Add/use helper in _BoundMetadataEntry?

https://codereview.chromium.org/1153243003/diff/30001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/metadata_collector.dart:281: print("UNUSED
${entry.hashCode}");
Remove debug print?

https://codereview.chromium.org/1153243003/diff/30001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/metadata_collector.dart:295: bool
isUsed(_BoundMetadataEntry entry) => entry._rc > 0;
Move this helper to _BoundMetaDataEntry?

https://codereview.chromium.org/1153243003/diff/30001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/metadata_collector.dart:342: print("UNBOUND
${outputUnit.name} ${token.debugData} ${token.debugData.hashCode}");
Remove debug print.

https://codereview.chromium.org/1153243003/diff/30001/pkg/compiler/lib/src/js...
File pkg/compiler/lib/src/js_emitter/model.dart (right):

https://codereview.chromium.org/1153243003/diff/30001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/model.dart:58: /// Getter for the list of
metadata entries for a given [OutputUnit].
Is is not a getter...

https://codereview.chromium.org/1153243003/diff/30001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/model.dart:68: =>
_metadataCollector.getTypesForOutputUnit(unit);
Use {} and return when it does not fit on one line.

https://codereview.chromium.org/1153243003/diff/30001/pkg/compiler/lib/src/js...
File pkg/compiler/lib/src/js_emitter/old_emitter/class_builder.dart (right):

https://codereview.chromium.org/1153243003/diff/30001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/old_emitter/class_builder.dart:42:
jsAst.ObjectInitializer toObjectInitializer(Compiler compiler,
The parameter 'compiler' seems to be unsued.

https://codereview.chromium.org/1153243003/diff/30001/pkg/compiler/lib/src/js...
File pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart (right):

https://codereview.chromium.org/1153243003/diff/30001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart:692:
parts..add(js.statement('# = #;', [metadataAccess, program.metadata]))
Consider adding type arguments when changing this code (though using no type
arguments seems to be the current style in this file).

https://codereview.chromium.org/1153243003/diff/30001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart:1272:
..add(js.comment(HOOKS_API_USAGE));
Revert IDE's change. (also next two changes below)

https://codereview.chromium.org/1153243003/diff/30001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart:1655: void
finalizeTokensInAst(jsAst.Program main, Iterable<jsAst.Program> parts) {
'parts' -> 'deferredParts'? I think it feels strange to have main not included
in a list called 'parts'.

https://codereview.chromium.org/1153243003/diff/30001/pkg/compiler/lib/src/ss...
File pkg/compiler/lib/src/ssa/optimize.dart (right):

https://codereview.chromium.org/1153243003/diff/30001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/optimize.dart:1040: JavaScriptBackend get backend =>
compiler.backend;
Is this getter used?

https://codereview.chromium.org/1153243003/diff/30001/pkg/compiler/lib/src/ss...
File pkg/compiler/lib/src/ssa/ssa.dart (right):

https://codereview.chromium.org/1153243003/diff/30001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/ssa.dart:8: import 'dart:mirrors' as mirror;
Is this import used?

[Johnni Winther, 2015-05-28 12:07:17]

Constants LGTM

https://codereview.chromium.org/1153243003/diff/30001/pkg/compiler/lib/src/co...
File pkg/compiler/lib/src/constants/expressions.dart (right):

https://codereview.chromium.org/1153243003/diff/30001/pkg/compiler/lib/src/co...
pkg/compiler/lib/src/constants/expressions.dart:339: class
DummyConstantExpression extends ConstantExpression {
Add a TODO for me to see if we can avoid the need for this.

https://codereview.chromium.org/1153243003/diff/30001/pkg/compiler/lib/src/co...
File pkg/compiler/lib/src/constants/values.dart (right):

https://codereview.chromium.org/1153243003/diff/30001/pkg/compiler/lib/src/co...
pkg/compiler/lib/src/constants/values.dart:606: class DummyConstantValue extends
ConstantValue {
Rename to SyntheticConstantValue, ForeignConstantValue or BackendConstantValue,
or ...

[herhut, 2015-06-01 12:09:43]

First round of comments addressed.

https://codereview.chromium.org/1153243003/diff/30001/pkg/compiler/lib/src/co...
File pkg/compiler/lib/src/constants/expressions.dart (right):

https://codereview.chromium.org/1153243003/diff/30001/pkg/compiler/lib/src/co...
pkg/compiler/lib/src/constants/expressions.dart:339: class
DummyConstantExpression extends ConstantExpression {
On 2015/05/28 12:07:17, Johnni Winther wrote:
> Add a TODO for me to see if we can avoid the need for this.

Done.

https://codereview.chromium.org/1153243003/diff/30001/pkg/compiler/lib/src/co...
File pkg/compiler/lib/src/constants/values.dart (right):

https://codereview.chromium.org/1153243003/diff/30001/pkg/compiler/lib/src/co...
pkg/compiler/lib/src/constants/values.dart:606: class DummyConstantValue extends
ConstantValue {
On 2015/05/28 12:07:17, Johnni Winther wrote:
> Rename to SyntheticConstantValue, ForeignConstantValue or
BackendConstantValue,
> or ...

I went for SyntheticConstantValue.

https://codereview.chromium.org/1153243003/diff/30001/pkg/compiler/lib/src/co...
pkg/compiler/lib/src/constants/values.dart:607: final payload;
On 2015/05/27 19:38:50, sra1 wrote:
> TL;DR: should this class be split into 2 to 3 classes?
> 
> The original dummy constant was only copied (e.g. passed as an argument to an
> unused parameter).  It did not matter at all what the textual JS
representation
> was.
> 
> If there is a different kind of 'constant' that actually flows to a nontrivial
> use I think it should be a different kind of ConstantValue.  When 'payload' is
> an expression, what how is that expression used at runtime?
> 
> I see that some uses of payload are js.Expression and some are TypeMask.  Are
> there other uses?

The idea is that these constants are opaque to the constant system and only the
specific system that uses them, e.g. the JavaScript backend, knows which exist
and what they are. 

Currently we have
  DUMMY_INTERCEPTOR
  EMPTY_VALUE
  TYPEVARIABLE_REFERENCE

Only the last is new, the other two existed before.

https://codereview.chromium.org/1153243003/diff/30001/pkg/compiler/lib/src/js...
File pkg/compiler/lib/src/js_backend/backend.dart (right):

https://codereview.chromium.org/1153243003/diff/30001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_backend/backend.dart:215: class DummyConstantKinds {
On 2015/05/28 09:39:52, karlklose wrote:
> I think it use sites of these constants read better if you called the class
> 'DummyConstantKind' and wrote the constants in all caps.

Done.

https://codereview.chromium.org/1153243003/diff/30001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_backend/backend.dart:218: static const String
typeVariableReference = "TYPEVARIALE_REFERENCE";
On 2015/05/27 14:26:41, sigurdm wrote:
> DBC: Typo in string

Done.

https://codereview.chromium.org/1153243003/diff/30001/pkg/compiler/lib/src/js...
File pkg/compiler/lib/src/js_emitter/helpers.dart (right):

https://codereview.chromium.org/1153243003/diff/30001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/helpers.dart:22: void setHash(String hash) {
On 2015/05/28 09:39:52, karlklose wrote:
> Maybe this pattern could be extraced in a class 'Lazy<T>'? The output unit
seems
> to be here for debugging purposes only.

There is a tiny little bit of work hidden in here: It does add quotes around the
hash that is passed in. Also, I find the use sites so much nicer to read with a
dedicated class. For a Lazy<String> I would know less what is really stored.

I'll keep this in mind if more uses of this pattern arise.

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

https://codereview.chromium.org/1153243003/diff/30001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/metadata_collector.dart:10: int get _rc;
On 2015/05/28 09:39:53, karlklose wrote:
> Could you add comments for this class and _rc?

Done.

https://codereview.chromium.org/1153243003/diff/30001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/metadata_collector.dart:30: if (!isFinalized)
print ("OHA! I have no value!");
On 2015/05/28 09:39:53, karlklose wrote:
> Remove/replace debug print.

Done.

https://codereview.chromium.org/1153243003/diff/30001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/metadata_collector.dart:60: return new
jsAst.LiteralString('"BOGUS: $debugData"');
On 2015/05/28 09:39:53, karlklose wrote:
> assert(isBound) or throw?

Done.

https://codereview.chromium.org/1153243003/diff/30001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/metadata_collector.dart:65: return -336699;
On 2015/05/27 19:38:51, sra1 wrote:
> Should this be
>   assert(isBound);
> ?

Done.

https://codereview.chromium.org/1153243003/diff/30001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/metadata_collector.dart:211: Placeholder
getMetadataPlaceholder([debug]) => new _ForwardingMetadataEntry(debug);
On 2015/05/27 19:38:50, sra1 wrote:
> line length

Done.

https://codereview.chromium.org/1153243003/diff/30001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/metadata_collector.dart:280: if (entry._rc == 0)
{
On 2015/05/28 09:39:53, karlklose wrote:
> Add/use helper in _BoundMetadataEntry?

Done.

https://codereview.chromium.org/1153243003/diff/30001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/metadata_collector.dart:281: print("UNUSED
${entry.hashCode}");
On 2015/05/28 09:39:53, karlklose wrote:
> Remove debug print?

Done.

https://codereview.chromium.org/1153243003/diff/30001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/metadata_collector.dart:294:
jsAst.ArrayInitializer finalizeMap(Map<dynamic, _BoundMetadataEntry> map) {
On 2015/05/27 19:38:50, sra1 wrote:
> Indentation

Done.

https://codereview.chromium.org/1153243003/diff/30001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/metadata_collector.dart:295: bool
isUsed(_BoundMetadataEntry entry) => entry._rc > 0;
On 2015/05/28 09:39:53, karlklose wrote:
> Move this helper to _BoundMetaDataEntry?

Done.

https://codereview.chromium.org/1153243003/diff/30001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/metadata_collector.dart:297: entries.sort();
On 2015/05/27 19:38:50, sra1 wrote:
> Sort is not stable.
> Equivalence classes by reference count will be ordered arbitrarily.

True but this is not a regression. The original implementation assigned them
first come first served. At least there is now a place where one could improve
stability.

https://codereview.chromium.org/1153243003/diff/30001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/metadata_collector.dart:302: }
On 2015/05/27 20:30:15, sra1 wrote:
> Longer term I think we need to be more sophisticated to control build-to-build
> differences.  We could bucket entries by number of digits and then allocate
> within the bucket by some deterministic feature-based allocation.  This would
> protect the large number of low reference-count entries being moved due to
> something being 'promoted' over them by an additional reference or two.

Acknowledged.

https://codereview.chromium.org/1153243003/diff/30001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/metadata_collector.dart:342: print("UNBOUND
${outputUnit.name} ${token.debugData} ${token.debugData.hashCode}");
On 2015/05/28 09:39:53, karlklose wrote:
> Remove debug print.

Turned the entire thing into an assertion.

https://codereview.chromium.org/1153243003/diff/30001/pkg/compiler/lib/src/js...
File pkg/compiler/lib/src/js_emitter/model.dart (right):

https://codereview.chromium.org/1153243003/diff/30001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/model.dart:58: /// Getter for the list of
metadata entries for a given [OutputUnit].
On 2015/05/28 09:39:53, karlklose wrote:
> Is is not a getter...

Done.

https://codereview.chromium.org/1153243003/diff/30001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/model.dart:68: =>
_metadataCollector.getTypesForOutputUnit(unit);
On 2015/05/28 09:39:53, karlklose wrote:
> Use {} and return when it does not fit on one line.

Agree, I inherited this.

https://codereview.chromium.org/1153243003/diff/30001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/model.dart:238: final js.Expression
functionTypeIndex;
On 2015/05/27 19:38:51, sra1 wrote:
> Could this be js.TokenNumber ?

I don't want to expose the fact that the result is a special thing outside of
the MetadataCollector. This code is not supposed to do anything with it.

https://codereview.chromium.org/1153243003/diff/30001/pkg/compiler/lib/src/js...
File pkg/compiler/lib/src/js_emitter/old_emitter/class_builder.dart (right):

https://codereview.chromium.org/1153243003/diff/30001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/old_emitter/class_builder.dart:42:
jsAst.ObjectInitializer toObjectInitializer(Compiler compiler,
On 2015/05/28 09:39:53, karlklose wrote:
> The parameter 'compiler' seems to be unsued.

True. Removed.

https://codereview.chromium.org/1153243003/diff/30001/pkg/compiler/lib/src/js...
File pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart (right):

https://codereview.chromium.org/1153243003/diff/30001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart:692:
parts..add(js.statement('# = #;', [metadataAccess, program.metadata]))
On 2015/05/28 09:39:53, karlklose wrote:
> Consider adding type arguments when changing this code (though using no type
> arguments seems to be the current style in this file).

Acknowledged.

https://codereview.chromium.org/1153243003/diff/30001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart:1272:
..add(js.comment(HOOKS_API_USAGE));
On 2015/05/28 09:39:53, karlklose wrote:
> Revert IDE's change. (also next two changes below)

Done.

https://codereview.chromium.org/1153243003/diff/30001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart:1655: void
finalizeTokensInAst(jsAst.Program main, Iterable<jsAst.Program> parts) {
On 2015/05/28 09:39:53, karlklose wrote:
> 'parts' -> 'deferredParts'? I think it feels strange to have main not included
> in a list called 'parts'.

Done.

https://codereview.chromium.org/1153243003/diff/30001/pkg/compiler/lib/src/js...
File pkg/compiler/lib/src/js_emitter/runtime_type_generator.dart (right):

https://codereview.chromium.org/1153243003/diff/30001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/runtime_type_generator.dart:15: jsAst.Expression
functionTypeIndex;
On 2015/05/27 19:38:51, sra1 wrote:
> Can this be a stronger type?

I on purpose do not expose more.

https://codereview.chromium.org/1153243003/diff/30001/pkg/compiler/lib/src/ss...
File pkg/compiler/lib/src/ssa/optimize.dart (right):

https://codereview.chromium.org/1153243003/diff/30001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/optimize.dart:1040: JavaScriptBackend get backend =>
compiler.backend;
On 2015/05/28 09:39:53, karlklose wrote:
> Is this getter used?

Thanks! Not anymore...

https://codereview.chromium.org/1153243003/diff/30001/pkg/compiler/lib/src/ss...
File pkg/compiler/lib/src/ssa/ssa.dart (right):

https://codereview.chromium.org/1153243003/diff/30001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/ssa.dart:8: import 'dart:mirrors' as mirror;
On 2015/05/28 09:39:53, karlklose wrote:
> Is this import used?

Done.

https://codereview.chromium.org/1153243003/diff/30001/pkg/js_ast/lib/src/buil...
File pkg/js_ast/lib/src/builder.dart (right):

https://codereview.chromium.org/1153243003/diff/30001/pkg/js_ast/lib/src/buil...
pkg/js_ast/lib/src/builder.dart:341: _parts = <Literal>[stringPart('"')]
On 2015/05/27 20:30:15, sra1 wrote:
> This stringPart can be shared between begining and end, or over all
> concatenateStrings.

I share them locally for now.

https://codereview.chromium.org/1153243003/diff/30001/pkg/js_ast/lib/src/temp...
File pkg/js_ast/lib/src/template.dart (right):

https://codereview.chromium.org/1153243003/diff/30001/pkg/js_ast/lib/src/temp...
pkg/js_ast/lib/src/template.dart:657: Instantiator
visitTokenExpression(TokenExpression node) =>
On 2015/05/27 19:38:51, sra1 wrote:
> => same;

Neat!

https://codereview.chromium.org/1153243003/diff/30001/pkg/js_ast/lib/src/temp...
pkg/js_ast/lib/src/template.dart:660: Instantiator visitTokenNumber(TokenNumber
node) =>
On 2015/05/27 19:38:51, sra1 wrote:
> ditto

Done.

[herhut, 2015-06-09 10:19:01]

Patchset #4 (id:70001) has been deleted

[herhut, 2015-06-09 11:10:01]

addressed js_ast comments.

https://codereview.chromium.org/1153243003/diff/30001/pkg/js_ast/lib/src/node...
File pkg/js_ast/lib/src/nodes.dart (right):

https://codereview.chromium.org/1153243003/diff/30001/pkg/js_ast/lib/src/node...
pkg/js_ast/lib/src/nodes.dart:933: abstract class Token extends Literal {
On 2015/05/27 19:38:51, sra1 wrote:
> These are abstract classes that are overridden.
> Describe how they should be used.
> 
> I think 'DeferredToken' and 'DeferredNumberToken' etc would be a better names.

> 'Late' would be an acceptable synonym for 'Deferred'

Well, this is only one use (the current use) of the Token interface. Once could
also use it to insert a blob into the AST without telling what it really is.
Just as an example. But I am fine with the renaming if you want to narrow the
scope.

https://codereview.chromium.org/1153243003/diff/30001/pkg/js_ast/lib/src/node...
pkg/js_ast/lib/src/nodes.dart:951: abstract class TokenExpression extends Token
{
On 2015/05/27 19:38:51, sra1 wrote:
> What is a TokenExpression?
> DeferredExpression?

Done.

https://codereview.chromium.org/1153243003/diff/30001/pkg/js_ast/lib/src/node...
pkg/js_ast/lib/src/nodes.dart:954: Expression get value;
On 2015/05/27 19:38:51, sra1 wrote:
> What is the precedence?
> PRIMARY is almost certainly wrong.

It actually was correct for the current usage but I have changed Token to extend
Expression, so now any class that implements TokenExpression will have to supply
a precedence.

https://codereview.chromium.org/1153243003/diff/30001/pkg/js_ast/lib/src/prin...
File pkg/js_ast/lib/src/printer.dart (right):

https://codereview.chromium.org/1153243003/diff/30001/pkg/js_ast/lib/src/prin...
pkg/js_ast/lib/src/printer.dart:957: node.value.accept(this);
On 2015/05/27 19:38:51, sra1 wrote:
> Either the precedence needs to be fixed or this code needs to account for
> 'incorrect' precedence.

Done.

https://codereview.chromium.org/1153243003/diff/30001/pkg/js_ast/lib/src/prin...
pkg/js_ast/lib/src/printer.dart:965: out(" ");
On 2015/05/27 20:30:15, sra1 wrote:
> I'd like to see a test that covers this case.

I have factored out the logic from visitLiteralNumber (where this came from)
into a shared helper. So the actual code will be tested by the tests for
LiteralNumber and no special test for the deferred version is needed.

[sra1, 2015-06-10 19:30:00]

lgtm

https://chromiumcodereview.appspot.com/1153243003/diff/110001/pkg/compiler/li...
File pkg/compiler/lib/src/js_backend/backend.dart (right):

https://chromiumcodereview.appspot.com/1153243003/diff/110001/pkg/compiler/li...
pkg/compiler/lib/src/js_backend/backend.dart:216: static const String
DUMMY_RECEIVER = "DUMMY_INTERCEPTOR";
Should this be an enum?

https://chromiumcodereview.appspot.com/1153243003/diff/110001/pkg/compiler/li...
File pkg/compiler/lib/src/ssa/types_propagation.dart (right):

https://chromiumcodereview.appspot.com/1153243003/diff/110001/pkg/compiler/li...
pkg/compiler/lib/src/ssa/types_propagation.dart:150: print("here");
Remove debugging code

https://chromiumcodereview.appspot.com/1153243003/diff/130001/pkg/js_ast/lib/...
File pkg/js_ast/lib/src/builder.dart (right):

https://chromiumcodereview.appspot.com/1153243003/diff/130001/pkg/js_ast/lib/...
pkg/js_ast/lib/src/builder.dart:1398: if (isNextSeparator) return
separator._clone();
Why clone?
_clone() is really just an implementation detail for attaching source info.  I'd
rather it didn't get used for anything else.

https://chromiumcodereview.appspot.com/1153243003/diff/130001/pkg/js_ast/lib/...
File pkg/js_ast/lib/src/nodes.dart (right):

https://chromiumcodereview.appspot.com/1153243003/diff/130001/pkg/js_ast/lib/...
pkg/js_ast/lib/src/nodes.dart:936: /// at construction time of an ast. Other
than [InterpolatedExpression],
'Other than' -> 'Unlike' ?

https://chromiumcodereview.appspot.com/1153243003/diff/130001/pkg/js_ast/lib/...
File pkg/js_ast/lib/src/template.dart (right):

https://chromiumcodereview.appspot.com/1153243003/diff/130001/pkg/js_ast/lib/...
pkg/js_ast/lib/src/template.dart:682: .map((Instantiator instantiator) =>
instantiator(arguments))
This is fine.

I was wondering if you saw a use for parsing an StringConcatenation with
embedded placeholders (which could then splice list arguments)?

[herhut, 2015-06-11 08:03:55]

https://chromiumcodereview.appspot.com/1153243003/diff/110001/pkg/compiler/li...
File pkg/compiler/lib/src/js_backend/backend.dart (right):

https://chromiumcodereview.appspot.com/1153243003/diff/110001/pkg/compiler/li...
pkg/compiler/lib/src/js_backend/backend.dart:216: static const String
DUMMY_RECEIVER = "DUMMY_INTERCEPTOR";
On 2015/06/10 19:29:59, sra1 wrote:
> Should this be an enum?

Done. Now that we have them, why not use them.

https://chromiumcodereview.appspot.com/1153243003/diff/110001/pkg/compiler/li...
File pkg/compiler/lib/src/ssa/types_propagation.dart (right):

https://chromiumcodereview.appspot.com/1153243003/diff/110001/pkg/compiler/li...
pkg/compiler/lib/src/ssa/types_propagation.dart:150: print("here");
On 2015/06/10 19:29:59, sra1 wrote:
> Remove debugging code

Already removed in the cleanup patch that followed this one.

https://chromiumcodereview.appspot.com/1153243003/diff/130001/pkg/js_ast/lib/...
File pkg/js_ast/lib/src/builder.dart (right):

https://chromiumcodereview.appspot.com/1153243003/diff/130001/pkg/js_ast/lib/...
pkg/js_ast/lib/src/builder.dart:1398: if (isNextSeparator) return
separator._clone();
On 2015/06/10 19:30:00, sra1 wrote:
> Why clone?
> _clone() is really just an implementation detail for attaching source info. 
I'd
> rather it didn't get used for anything else.

Done.

https://chromiumcodereview.appspot.com/1153243003/diff/130001/pkg/js_ast/lib/...
File pkg/js_ast/lib/src/nodes.dart (right):

https://chromiumcodereview.appspot.com/1153243003/diff/130001/pkg/js_ast/lib/...
pkg/js_ast/lib/src/nodes.dart:936: /// at construction time of an ast. Other
than [InterpolatedExpression],
On 2015/06/10 19:30:00, sra1 wrote:
> 'Other than' -> 'Unlike' ?

Done.

https://chromiumcodereview.appspot.com/1153243003/diff/130001/pkg/js_ast/lib/...
File pkg/js_ast/lib/src/template.dart (right):

https://chromiumcodereview.appspot.com/1153243003/diff/130001/pkg/js_ast/lib/...
pkg/js_ast/lib/src/template.dart:682: .map((Instantiator instantiator) =>
instantiator(arguments))
On 2015/06/10 19:30:00, sra1 wrote:
> This is fine.
> 
> I was wondering if you saw a use for parsing an StringConcatenation with
> embedded placeholders (which could then splice list arguments)?

I only use them in a very limited form but wanted to make sure that they do not
have unsurprising behaviour in the presence of placeholders.

[herhut, 2015-06-11 08:13:38]

Committed patchset #7 (id:150001) manually as
e0e96624b95d6a189af6288012b8a8d5fc78f5b3 (presubmit successful).