Adding package:code_transformers for unifying common transformers code

Adding package:code_transformers for unifying common transformers code

There's quite a bit of code which would be great to share between Polymer and Angular, adding this as the starting point for some of that code.

This CL is primarily a transformer that resolves the AST which can be used by other transformers when modifying code.

BUG=
R=sigmund@google.com

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

[blois, 2014-02-13 01:29:28]

https://codereview.chromium.org/140203007/diff/10014/pkg/code_transformers/li...
File pkg/code_transformers/lib/src/dart_sdk.dart (right):

https://codereview.chromium.org/140203007/diff/10014/pkg/code_transformers/li...
pkg/code_transformers/lib/src/dart_sdk.dart:19: var result =
Process.runSync('which', ['dart'],
Still working on this- this codepath is hit when tests are executed from dart
directly, and dart is on the environment path.

[Siggi Cherem (dart-lang), 2014-02-14 02:13:37]

really cool Pete. Bunch of minor comments.

https://codereview.chromium.org/140203007/diff/10014/pkg/code_transformers/li...
File pkg/code_transformers/lib/src/resolver.dart (right):

https://codereview.chromium.org/140203007/diff/10014/pkg/code_transformers/li...
pkg/code_transformers/lib/src/resolver.dart:37: /// [uri] must be an absolute
URI of the form [dart:|package:]path/file.dart.
not sure what our dartdoc does here with the brackets around [dart:|package:],
maybe enclose this in `backticks` as well?

https://codereview.chromium.org/140203007/diff/10014/pkg/code_transformers/li...
pkg/code_transformers/lib/src/resolver.dart:42: /// This will resolve the first
instance of this, because of potential library
of this => of it? or "of this type" (just lost track that 'this' is, since the
first one refers to this method)

https://codereview.chromium.org/140203007/diff/10014/pkg/code_transformers/li...
pkg/code_transformers/lib/src/resolver.dart:46: /// Resolves a fully-qualified
top-level library variable (library_name.Name).
Name => variableName (to match variable-naming conventions too)

https://codereview.chromium.org/140203007/diff/10014/pkg/code_transformers/li...
pkg/code_transformers/lib/src/resolver.dart:48: /// This will resolve the first
instance of this, because of potential library
same here, and below

https://codereview.chromium.org/140203007/diff/10014/pkg/code_transformers/li...
pkg/code_transformers/lib/src/resolver.dart:52: /// Resolves a fully-qualified
top-level library function (library_name.Name).
Name -> functionName

https://codereview.chromium.org/140203007/diff/10014/pkg/code_transformers/li...
pkg/code_transformers/lib/src/resolver.dart:72: /// to be edited, returns null
otherwise.
maybe indicate what does this transaction contain? it it basically the text from
the element-begin-offset to element-end-offset?

https://codereview.chromium.org/140203007/diff/10014/pkg/code_transformers/li...
pkg/code_transformers/lib/src/resolver.dart:75: 
nit: delete trailing lines

https://codereview.chromium.org/140203007/diff/10014/pkg/code_transformers/li...
File pkg/code_transformers/lib/src/resolver_impl.dart (right):

https://codereview.chromium.org/140203007/diff/10014/pkg/code_transformers/li...
pkg/code_transformers/lib/src/resolver_impl.dart:33: /// The Dart entry point
file where parsing begins.
nit: add empty line above comment

https://codereview.chromium.org/140203007/diff/10014/pkg/code_transformers/li...
pkg/code_transformers/lib/src/resolver_impl.dart:41: /// The currently resolved
library, or null if unresolved.
same here and below

https://codereview.chromium.org/140203007/diff/10014/pkg/code_transformers/li...
pkg/code_transformers/lib/src/resolver_impl.dart:54: ..cacheSize = 256 // # of
sources to cache ASTs for.
can we make it bigger :)?

https://codereview.chromium.org/140203007/diff/10014/pkg/code_transformers/li...
pkg/code_transformers/lib/src/resolver_impl.dart:56: ..analyzeFunctionBodies =
true;
should this be an option too? Seems like a common use for the resolver can be
without this

https://codereview.chromium.org/140203007/diff/10014/pkg/code_transformers/li...
pkg/code_transformers/lib/src/resolver_impl.dart:96: var assetId =
toVisit.first;
might be cheaper to use a queue instead of a Set for toVisit

https://codereview.chromium.org/140203007/diff/10014/pkg/code_transformers/li...
pkg/code_transformers/lib/src/resolver_impl.dart:118: return visitNext();
FYI - this is fine, but it might be worth using FutureGroup in the future for
this (I have similar code in polymer I'd like to update).

FutureGroup works great for this kind of work where you dynamically and
asynchronously enqueue more work to do.

https://codereview.chromium.org/140203007/diff/10014/pkg/code_transformers/li...
pkg/code_transformers/lib/src/resolver_impl.dart:128: ChangeSet changeSet = new
ChangeSet();
-> var

https://codereview.chromium.org/140203007/diff/10014/pkg/code_transformers/li...
pkg/code_transformers/lib/src/resolver_impl.dart:212: path: 
path.relative(id.path, from: path.dirname(from.path)));
remove extra space

https://codereview.chromium.org/140203007/diff/10014/pkg/code_transformers/li...
pkg/code_transformers/lib/src/resolver_impl.dart:233: var sourceFile = new
SourceFile.text(assetId.path,
let's change this first argument to match what we do in
polymer/src/build/common.dart#spanUrlFor(id, ...)

Basically, it's useful to have the package name in the url if you are processing
an element that it's not in the same package as the primary input. More details
on the #spanUrlFor comment here:
https://code.google.com/p/dart/source/browse/branches/bleeding_edge/dart/pkg/...

https://codereview.chromium.org/140203007/diff/10014/pkg/code_transformers/li...
pkg/code_transformers/lib/src/resolver_impl.dart:245: var sourceFile = new
SourceFile.text(source.assetId.path, source.contents);
same here

https://codereview.chromium.org/140203007/diff/10014/pkg/code_transformers/li...
pkg/code_transformers/lib/src/resolver_impl.dart:254: /// The resolver this is
being used in.
similar style nit here: add empty line above each comment block

https://codereview.chromium.org/140203007/diff/10014/pkg/code_transformers/li...
pkg/code_transformers/lib/src/resolver_impl.dart:271: ++_revision;
_revision++?

It's style nit, I think we use i++ more than ++i, it probably will read better
because of the _ too.

https://codereview.chromium.org/140203007/diff/10014/pkg/code_transformers/li...
pkg/code_transformers/lib/src/resolver_impl.dart:288: Iterable<AssetId> get
dependentAssets {
since you already have a cache of _AssetBasedSource up in the ResolverImpl, it
might be worth storing the dependentAssets explicitly. Then invalidate the cache
when you call updateContents and find that the contents actually changed.

https://codereview.chromium.org/140203007/diff/10014/pkg/code_transformers/li...
pkg/code_transformers/lib/src/resolver_impl.dart:353: SourceFile get _sourceFile
=> new SourceFile.text(assetId.path, contents);
same here: use something like spanUriFor(...)

https://codereview.chromium.org/140203007/diff/10014/pkg/code_transformers/li...
pkg/code_transformers/lib/src/resolver_impl.dart:419: /// Underlying source
object.
+line

https://codereview.chromium.org/140203007/diff/10014/pkg/code_transformers/li...
pkg/code_transformers/lib/src/resolver_impl.dart:467: AssetId _resolve(AssetId
source, String url, TransformLogger logger,
this seems similar to what we have in common, is the idea to unify them? I'm ok
not unifying them now, but maybe add a TODO?

https://codereview.chromium.org/140203007/diff/10014/pkg/code_transformers/li...
pkg/code_transformers/lib/src/resolver_impl.dart:480: if (uri.scheme == 'dart')
{
style nit: fast-exit style

if (uri.scheme == 'dart') return null;

https://codereview.chromium.org/140203007/diff/10014/pkg/code_transformers/li...
File pkg/code_transformers/lib/src/resolver_transformer.dart (right):

https://codereview.chromium.org/140203007/diff/10014/pkg/code_transformers/li...
pkg/code_transformers/lib/src/resolver_transformer.dart:33: final Map<AssetId,
Resolver> _resolvers = {};
why not use the same resolver for all entrypoints?

In the current form, if there is a shared library between main1.dart and
main2.dart, would this resolve the shared library twice?

https://codereview.chromium.org/140203007/diff/10014/pkg/code_transformers/li...
File pkg/code_transformers/lib/tests.dart (right):

https://codereview.chromium.org/140203007/diff/10014/pkg/code_transformers/li...
pkg/code_transformers/lib/tests.dart:5: library code_transformers.tests;
might be worth adding a library comment for this one too (maybe the comment you
had in the test_harness file)

https://codereview.chromium.org/140203007/diff/10014/pkg/code_transformers/li...
pkg/code_transformers/lib/tests.dart:14: /// Defines a test which invokes
[updateTransformers]
minor nit: add a '.' at the end

https://codereview.chromium.org/140203007/diff/10014/pkg/code_transformers/li...
pkg/code_transformers/lib/tests.dart:31: Future
updateTransformers(List<List<Transformer>> phases,
mmm, normally I don't think of transformers as being updated. How about
"runTrunsformers" or "runPhases"?

https://codereview.chromium.org/140203007/diff/10014/pkg/code_transformers/pu...
File pkg/code_transformers/pubspec.yaml (right):

https://codereview.chromium.org/140203007/diff/10014/pkg/code_transformers/pu...
pkg/code_transformers/pubspec.yaml:4: description: Collection of utilities
related to creating Pub transformers.
Pub => barback?

https://codereview.chromium.org/140203007/diff/10014/pkg/code_transformers/te...
File pkg/code_transformers/test/resolver_test.dart (right):

https://codereview.chromium.org/140203007/diff/10014/pkg/code_transformers/te...
pkg/code_transformers/test/resolver_test.dart:62: 'a|web/a.dart': '''
unindent (here and in other tests below)

[blois, 2014-02-19 21:42:57]

https://codereview.chromium.org/140203007/diff/10014/pkg/code_transformers/li...
File pkg/code_transformers/lib/src/resolver.dart (right):

https://codereview.chromium.org/140203007/diff/10014/pkg/code_transformers/li...
pkg/code_transformers/lib/src/resolver.dart:37: /// [uri] must be an absolute
URI of the form [dart:|package:]path/file.dart.
On 2014/02/14 02:13:37, Siggi Cherem (dart-lang) wrote:
> not sure what our dartdoc does here with the brackets around [dart:|package:],
> maybe enclose this in `backticks` as well?

Done.

https://codereview.chromium.org/140203007/diff/10014/pkg/code_transformers/li...
pkg/code_transformers/lib/src/resolver.dart:42: /// This will resolve the first
instance of this, because of potential library
On 2014/02/14 02:13:37, Siggi Cherem (dart-lang) wrote:
> of this => of it? or "of this type" (just lost track that 'this' is, since the
> first one refers to this method)

Done.

https://codereview.chromium.org/140203007/diff/10014/pkg/code_transformers/li...
pkg/code_transformers/lib/src/resolver.dart:46: /// Resolves a fully-qualified
top-level library variable (library_name.Name).
On 2014/02/14 02:13:37, Siggi Cherem (dart-lang) wrote:
> Name => variableName (to match variable-naming conventions too)

Done.

https://codereview.chromium.org/140203007/diff/10014/pkg/code_transformers/li...
pkg/code_transformers/lib/src/resolver.dart:48: /// This will resolve the first
instance of this, because of potential library
On 2014/02/14 02:13:37, Siggi Cherem (dart-lang) wrote:
> same here, and below

Done.

https://codereview.chromium.org/140203007/diff/10014/pkg/code_transformers/li...
pkg/code_transformers/lib/src/resolver.dart:52: /// Resolves a fully-qualified
top-level library function (library_name.Name).
On 2014/02/14 02:13:37, Siggi Cherem (dart-lang) wrote:
> Name -> functionName

Done.

https://codereview.chromium.org/140203007/diff/10014/pkg/code_transformers/li...
pkg/code_transformers/lib/src/resolver.dart:72: /// to be edited, returns null
otherwise.
On 2014/02/14 02:13:37, Siggi Cherem (dart-lang) wrote:
> maybe indicate what does this transaction contain? it it basically the text
from
> the element-begin-offset to element-end-offset?

Done.

https://codereview.chromium.org/140203007/diff/10014/pkg/code_transformers/li...
pkg/code_transformers/lib/src/resolver.dart:75: 
On 2014/02/14 02:13:37, Siggi Cherem (dart-lang) wrote:
> nit: delete trailing lines

Done.

https://codereview.chromium.org/140203007/diff/10014/pkg/code_transformers/li...
File pkg/code_transformers/lib/src/resolver_impl.dart (right):

https://codereview.chromium.org/140203007/diff/10014/pkg/code_transformers/li...
pkg/code_transformers/lib/src/resolver_impl.dart:33: /// The Dart entry point
file where parsing begins.
On 2014/02/14 02:13:37, Siggi Cherem (dart-lang) wrote:
> nit: add empty line above comment

Done.

https://codereview.chromium.org/140203007/diff/10014/pkg/code_transformers/li...
pkg/code_transformers/lib/src/resolver_impl.dart:41: /// The currently resolved
library, or null if unresolved.
On 2014/02/14 02:13:37, Siggi Cherem (dart-lang) wrote:
> same here and below

Done.

https://codereview.chromium.org/140203007/diff/10014/pkg/code_transformers/li...
pkg/code_transformers/lib/src/resolver_impl.dart:54: ..cacheSize = 256 // # of
sources to cache ASTs for.
On 2014/02/14 02:13:37, Siggi Cherem (dart-lang) wrote:
> can we make it bigger :)?

I actually just cloned this from Angular's extractor- can make it bigger but
it's just a perf optimization (AST will be re-generated if it is needed after
purged). Default is 64.

https://codereview.chromium.org/140203007/diff/10014/pkg/code_transformers/li...
pkg/code_transformers/lib/src/resolver_impl.dart:56: ..analyzeFunctionBodies =
true;
On 2014/02/14 02:13:37, Siggi Cherem (dart-lang) wrote:
> should this be an option too? Seems like a common use for the resolver can be
> without this

Added, but the options themselves can be provided as well.

https://codereview.chromium.org/140203007/diff/10014/pkg/code_transformers/li...
pkg/code_transformers/lib/src/resolver_impl.dart:96: var assetId =
toVisit.first;
On 2014/02/14 02:13:37, Siggi Cherem (dart-lang) wrote:
> might be cheaper to use a queue instead of a Set for toVisit

Since order is not critical better to optimize for the contains check?

https://codereview.chromium.org/140203007/diff/10014/pkg/code_transformers/li...
pkg/code_transformers/lib/src/resolver_impl.dart:118: return visitNext();
On 2014/02/14 02:13:37, Siggi Cherem (dart-lang) wrote:
> FYI - this is fine, but it might be worth using FutureGroup in the future for
> this (I have similar code in polymer I'd like to update).
> 
> FutureGroup works great for this kind of work where you dynamically and
> asynchronously enqueue more work to do.

Would prefer to leave as-is for now, though switching to FutureGroup in the
future may allow IO to be parallelized in the future.

https://codereview.chromium.org/140203007/diff/10014/pkg/code_transformers/li...
pkg/code_transformers/lib/src/resolver_impl.dart:128: ChangeSet changeSet = new
ChangeSet();
On 2014/02/14 02:13:37, Siggi Cherem (dart-lang) wrote:
> -> var

Done.

https://codereview.chromium.org/140203007/diff/10014/pkg/code_transformers/li...
pkg/code_transformers/lib/src/resolver_impl.dart:212: path: 
path.relative(id.path, from: path.dirname(from.path)));
On 2014/02/14 02:13:37, Siggi Cherem (dart-lang) wrote:
> remove extra space

Done.

https://codereview.chromium.org/140203007/diff/10014/pkg/code_transformers/li...
pkg/code_transformers/lib/src/resolver_impl.dart:233: var sourceFile = new
SourceFile.text(assetId.path,
On 2014/02/14 02:13:37, Siggi Cherem (dart-lang) wrote:
> let's change this first argument to match what we do in
> polymer/src/build/common.dart#spanUrlFor(id, ...)
> 
> Basically, it's useful to have the package name in the url if you are
processing
> an element that it's not in the same package as the primary input. More
details
> on the #spanUrlFor comment here:
>
https://code.google.com/p/dart/source/browse/branches/bleeding_edge/dart/pkg/...

Done.

https://codereview.chromium.org/140203007/diff/10014/pkg/code_transformers/li...
pkg/code_transformers/lib/src/resolver_impl.dart:245: var sourceFile = new
SourceFile.text(source.assetId.path, source.contents);
On 2014/02/14 02:13:37, Siggi Cherem (dart-lang) wrote:
> same here

Done.

https://codereview.chromium.org/140203007/diff/10014/pkg/code_transformers/li...
pkg/code_transformers/lib/src/resolver_impl.dart:254: /// The resolver this is
being used in.
On 2014/02/14 02:13:37, Siggi Cherem (dart-lang) wrote:
> similar style nit here: add empty line above each comment block

Done.

https://codereview.chromium.org/140203007/diff/10014/pkg/code_transformers/li...
pkg/code_transformers/lib/src/resolver_impl.dart:271: ++_revision;
On 2014/02/14 02:13:37, Siggi Cherem (dart-lang) wrote:
> _revision++?
> 
> It's style nit, I think we use i++ more than ++i, it probably will read better
> because of the _ too.

Old habits die hard. Prefix operator is sometimes faster (when dealing with C++
iterators).

https://codereview.chromium.org/140203007/diff/10014/pkg/code_transformers/li...
pkg/code_transformers/lib/src/resolver_impl.dart:353: SourceFile get _sourceFile
=> new SourceFile.text(assetId.path, contents);
On 2014/02/14 02:13:37, Siggi Cherem (dart-lang) wrote:
> same here: use something like spanUriFor(...)

Done.

https://codereview.chromium.org/140203007/diff/10014/pkg/code_transformers/li...
pkg/code_transformers/lib/src/resolver_impl.dart:419: /// Underlying source
object.
On 2014/02/14 02:13:37, Siggi Cherem (dart-lang) wrote:
> +line

Done.

https://codereview.chromium.org/140203007/diff/10014/pkg/code_transformers/li...
pkg/code_transformers/lib/src/resolver_impl.dart:467: AssetId _resolve(AssetId
source, String url, TransformLogger logger,
On 2014/02/14 02:13:37, Siggi Cherem (dart-lang) wrote:
> this seems similar to what we have in common, is the idea to unify them? I'm
ok
> not unifying them now, but maybe add a TODO?

It's actually slightly different- this goes from a dart: URI to an asset ID, the
one in common goes from an asset URI to an asset ID.

https://codereview.chromium.org/140203007/diff/10014/pkg/code_transformers/li...
pkg/code_transformers/lib/src/resolver_impl.dart:480: if (uri.scheme == 'dart')
{
On 2014/02/14 02:13:37, Siggi Cherem (dart-lang) wrote:
> style nit: fast-exit style
> 
> if (uri.scheme == 'dart') return null;

Done.

https://codereview.chromium.org/140203007/diff/10014/pkg/code_transformers/li...
File pkg/code_transformers/lib/src/resolver_transformer.dart (right):

https://codereview.chromium.org/140203007/diff/10014/pkg/code_transformers/li...
pkg/code_transformers/lib/src/resolver_transformer.dart:33: final Map<AssetId,
Resolver> _resolvers = {};
On 2014/02/14 02:13:37, Siggi Cherem (dart-lang) wrote:
> why not use the same resolver for all entrypoints?
> 
> In the current form, if there is a shared library between main1.dart and
> main2.dart, would this resolve the shared library twice?

Yes, it would resolve separately. Discussed some offline- it's useful to have a
single Resolver per entry point as there are a number of APIs on it which are
relative to the entry point (resolving types, getting import URIs, etc). It
should be possible to share the AnalysisContext between multiple resolvers,
though this would mean coordinating the added/removed sources & refactoring the
source to not have a reference to the resolver (both internal implementation
details).

https://codereview.chromium.org/140203007/diff/10014/pkg/code_transformers/li...
File pkg/code_transformers/lib/tests.dart (right):

https://codereview.chromium.org/140203007/diff/10014/pkg/code_transformers/li...
pkg/code_transformers/lib/tests.dart:5: library code_transformers.tests;
On 2014/02/14 02:13:37, Siggi Cherem (dart-lang) wrote:
> might be worth adding a library comment for this one too (maybe the comment
you
> had in the test_harness file)

Done.

https://codereview.chromium.org/140203007/diff/10014/pkg/code_transformers/li...
pkg/code_transformers/lib/tests.dart:14: /// Defines a test which invokes
[updateTransformers]
On 2014/02/14 02:13:37, Siggi Cherem (dart-lang) wrote:
> minor nit: add a '.' at the end

Done.

https://codereview.chromium.org/140203007/diff/10014/pkg/code_transformers/li...
pkg/code_transformers/lib/tests.dart:31: Future
updateTransformers(List<List<Transformer>> phases,
On 2014/02/14 02:13:37, Siggi Cherem (dart-lang) wrote:
> mmm, normally I don't think of transformers as being updated. How about
> "runTrunsformers" or "runPhases"? 

Done.

https://codereview.chromium.org/140203007/diff/10014/pkg/code_transformers/pu...
File pkg/code_transformers/pubspec.yaml (right):

https://codereview.chromium.org/140203007/diff/10014/pkg/code_transformers/pu...
pkg/code_transformers/pubspec.yaml:4: description: Collection of utilities
related to creating Pub transformers.
On 2014/02/14 02:13:37, Siggi Cherem (dart-lang) wrote:
> Pub => barback?

Done.

https://codereview.chromium.org/140203007/diff/10014/pkg/code_transformers/te...
File pkg/code_transformers/test/resolver_test.dart (right):

https://codereview.chromium.org/140203007/diff/10014/pkg/code_transformers/te...
pkg/code_transformers/test/resolver_test.dart:62: 'a|web/a.dart': '''
On 2014/02/14 02:13:37, Siggi Cherem (dart-lang) wrote:
> unindent (here and in other tests below)

Ack. Not sure how that happened!

[Siggi Cherem (dart-lang), 2014-02-19 22:36:05]

other than changing below the toVisit.contains, LGTM

https://codereview.chromium.org/140203007/diff/10014/pkg/code_transformers/li...
File pkg/code_transformers/lib/src/resolver_impl.dart (right):

https://codereview.chromium.org/140203007/diff/10014/pkg/code_transformers/li...
pkg/code_transformers/lib/src/resolver_impl.dart:56: ..analyzeFunctionBodies =
true;
On 2014/02/19 21:42:58, blois wrote:
> On 2014/02/14 02:13:37, Siggi Cherem (dart-lang) wrote:
> > should this be an option too? Seems like a common use for the resolver can
be
> > without this
> 
> Added, but the options themselves can be provided as well.

Ah - good point, I overlooked that if options is provided this codepath is not
reached, feel free to keep or remove it if you like.

https://codereview.chromium.org/140203007/diff/10014/pkg/code_transformers/li...
pkg/code_transformers/lib/src/resolver_impl.dart:96: var assetId =
toVisit.first;
On 2014/02/19 21:42:58, blois wrote:
> On 2014/02/14 02:13:37, Siggi Cherem (dart-lang) wrote:
> > might be cheaper to use a queue instead of a Set for toVisit
> 
> Since order is not critical better to optimize for the contains check?

oh - I think you can remove the check for 'toVisit.contains' and instead return
quickly here if it happened to have been visited:

  var assetId = toVisit.removeFirst();
  if (visited.contains(assetId)) return null;
  visited.add(assetId);

https://codereview.chromium.org/140203007/diff/10014/pkg/code_transformers/li...
pkg/code_transformers/lib/src/resolver_impl.dart:271: ++_revision;
On 2014/02/19 21:42:58, blois wrote:
> On 2014/02/14 02:13:37, Siggi Cherem (dart-lang) wrote:
> > _revision++?
> > 
> > It's style nit, I think we use i++ more than ++i, it probably will read
better
> > because of the _ too.
> 
> Old habits die hard. Prefix operator is sometimes faster (when dealing with
C++
> iterators).

Yeah, I recall that was the case in C++, good thing that's no longer true in
Dart :)

[blois, 2014-02-20 01:03:11]

https://codereview.chromium.org/140203007/diff/10014/pkg/code_transformers/li...
File pkg/code_transformers/lib/src/resolver_impl.dart (right):

https://codereview.chromium.org/140203007/diff/10014/pkg/code_transformers/li...
pkg/code_transformers/lib/src/resolver_impl.dart:96: var assetId =
toVisit.first;
On 2014/02/19 22:36:06, Siggi Cherem (dart-lang) wrote:
> On 2014/02/19 21:42:58, blois wrote:
> > On 2014/02/14 02:13:37, Siggi Cherem (dart-lang) wrote:
> > > might be cheaper to use a queue instead of a Set for toVisit
> > 
> > Since order is not critical better to optimize for the contains check?
> 
> oh - I think you can remove the check for 'toVisit.contains' and instead
return
> quickly here if it happened to have been visited:
> 
>   var assetId = toVisit.removeFirst();
>   if (visited.contains(assetId)) return null;
>   visited.add(assetId);

Ended up switching over to FutureGroup which allows removal of the toVisit set
completely.

[Siggi Cherem (dart-lang), 2014-02-20 02:21:09]

lgtm

https://codereview.chromium.org/140203007/diff/140002/pkg/code_transformers/l...
File pkg/code_transformers/lib/src/resolver_impl.dart (right):

https://codereview.chromium.org/140203007/diff/140002/pkg/code_transformers/l...
pkg/code_transformers/lib/src/resolver_impl.dart:493: // TODO(rnystrom): Copied
from web_components. Remove from here when it gets
might want to update the code to copy the latest from quiver (I thought we had
fixed some things there, so it might be worth checking) and maybe change the
TODO to indicate this is now available in quiver as well.

[blois, 2014-02-20 18:37:10]

Committed patchset #4 manually as r32859 (presubmit successful).