Make Assets know their ID.

Make Assets know their ID.

R=nweiz@google.com

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

[Bob Nystrom, 2013-07-03 18:09:36]

This isn't strictly necessary, but it cleans up some stuff and over time I think
it will be increasingly handy.

[nweiz, 2013-07-03 20:08:25]

https://codereview.chromium.org/18650004/diff/1/pkg/barback/lib/src/asset.dart
File pkg/barback/lib/src/asset.dart (right):

https://codereview.chromium.org/18650004/diff/1/pkg/barback/lib/src/asset.dar...
pkg/barback/lib/src/asset.dart:20: Asset._(this.id);
If this is no longer an interface, making this private will also prevent
external subclasses.

https://codereview.chromium.org/18650004/diff/1/pkg/barback/lib/src/asset_gra...
File pkg/barback/lib/src/asset_graph.dart (right):

https://codereview.chromium.org/18650004/diff/1/pkg/barback/lib/src/asset_gra...
pkg/barback/lib/src/asset_graph.dart:193: var updated = new Set<Asset>();
Does Set<Asset> work? Asset doesn't define [hash] or [operator==]. It would also
be weird to define those as forwarding to [AssetId], because then different
assets with the same id would register as equal.

It may be worth making a custom AssetSet class that wraps a Map<AssetId, Asset>.
That's a pattern that comes up a lot in the code, and it would be nice to have a
class that explicitly expresses "this is a set that's expected to contain only
one asset of a given id" without making the equality semantics of Asset weird.

https://codereview.chromium.org/18650004/diff/1/pkg/barback/lib/src/asset_nod...
File pkg/barback/lib/src/asset_node.dart (right):

https://codereview.chromium.org/18650004/diff/1/pkg/barback/lib/src/asset_nod...
pkg/barback/lib/src/asset_node.dart:19: class AssetNode {
Is this class really doing anything now? Couldn't you just have the
[updateAsset] logic built in to Phase?

https://codereview.chromium.org/18650004/diff/1/pkg/barback/lib/src/phase.dart
File pkg/barback/lib/src/phase.dart (right):

https://codereview.chromium.org/18650004/diff/1/pkg/barback/lib/src/phase.dar...
pkg/barback/lib/src/phase.dart:45: final inputs = new Map<AssetId, AssetNode>();
If you get rid of AssetNode, this could just be an AssetSet.

https://codereview.chromium.org/18650004/diff/1/pkg/barback/lib/src/phase.dar...
pkg/barback/lib/src/phase.dart:152: var updatedIds = new Set<AssetId>();
Another good place for AssetSet.

https://codereview.chromium.org/18650004/diff/1/pkg/barback/lib/src/transform...
File pkg/barback/lib/src/transform.dart (right):

https://codereview.chromium.org/18650004/diff/1/pkg/barback/lib/src/transform...
pkg/barback/lib/src/transform.dart:77: void addOutput(Asset output) {
This should throw an error if the transformer outputs the same asset id multiple
times. A TODO is fine.

[Bob Nystrom, 2013-07-03 22:32:11]

Thanks!

https://codereview.chromium.org/18650004/diff/1/pkg/barback/lib/src/asset.dart
File pkg/barback/lib/src/asset.dart (right):

https://codereview.chromium.org/18650004/diff/1/pkg/barback/lib/src/asset.dar...
pkg/barback/lib/src/asset.dart:20: Asset._(this.id);
On 2013/07/03 20:08:25, nweiz wrote:
> If this is no longer an interface, making this private will also prevent
> external subclasses.

Changed to public.

https://codereview.chromium.org/18650004/diff/1/pkg/barback/lib/src/asset_gra...
File pkg/barback/lib/src/asset_graph.dart (right):

https://codereview.chromium.org/18650004/diff/1/pkg/barback/lib/src/asset_gra...
pkg/barback/lib/src/asset_graph.dart:193: var updated = new Set<Asset>();
On 2013/07/03 20:08:25, nweiz wrote:
> Does Set<Asset> work? Asset doesn't define [hash] or [operator==].

Object implements hashCode and == (based on identity), so all objects can be put
in a set. Asset could generate a more appropriate/faster hashCode based on the
ID, but it should work regardless.

> It would also
> be weird to define those as forwarding to [AssetId], because then different
> assets with the same id would register as equal.

If you implement ==, you must implement hashCode, but the converse isn't
necessarily true. It's OK to have your own optimized hashCode but still fallback
on identity for equality.

> It may be worth making a custom AssetSet class that wraps a Map<AssetId,
Asset>.
> That's a pattern that comes up a lot in the code, and it would be nice to have
a
> class that explicitly expresses "this is a set that's expected to contain only
> one asset of a given id" without making the equality semantics of Asset weird.

Great idea. Done.

https://codereview.chromium.org/18650004/diff/1/pkg/barback/lib/src/asset_nod...
File pkg/barback/lib/src/asset_node.dart (right):

https://codereview.chromium.org/18650004/diff/1/pkg/barback/lib/src/asset_nod...
pkg/barback/lib/src/asset_node.dart:19: class AssetNode {
On 2013/07/03 20:08:25, nweiz wrote:
> Is this class really doing anything now? Couldn't you just have the
> [updateAsset] logic built in to Phase?

It keeps track of the set of transforms that depend on this node's asset as an
input.

https://codereview.chromium.org/18650004/diff/1/pkg/barback/lib/src/phase.dart
File pkg/barback/lib/src/phase.dart (right):

https://codereview.chromium.org/18650004/diff/1/pkg/barback/lib/src/phase.dar...
pkg/barback/lib/src/phase.dart:45: final inputs = new Map<AssetId, AssetNode>();
On 2013/07/03 20:08:25, nweiz wrote:
> If you get rid of AssetNode, this could just be an AssetSet.

See previous comment.

https://codereview.chromium.org/18650004/diff/1/pkg/barback/lib/src/phase.dar...
pkg/barback/lib/src/phase.dart:152: var updatedIds = new Set<AssetId>();
On 2013/07/03 20:08:25, nweiz wrote:
> Another good place for AssetSet.

Done.

https://codereview.chromium.org/18650004/diff/1/pkg/barback/lib/src/transform...
File pkg/barback/lib/src/transform.dart (right):

https://codereview.chromium.org/18650004/diff/1/pkg/barback/lib/src/transform...
pkg/barback/lib/src/transform.dart:77: void addOutput(Asset output) {
On 2013/07/03 20:08:25, nweiz wrote:
> This should throw an error if the transformer outputs the same asset id
multiple
> times. A TODO is fine.

Done.

[nweiz, 2013-07-03 22:58:33]

https://codereview.chromium.org/18650004/diff/10001/pkg/barback/lib/src/asset...
File pkg/barback/lib/src/asset_set.dart (right):

https://codereview.chromium.org/18650004/diff/10001/pkg/barback/lib/src/asset...
pkg/barback/lib/src/asset_set.dart:47: }
If [contains] has a different notion of equality than the rest of the class,
this shouldn't implement Set.

https://codereview.chromium.org/18650004/diff/10001/pkg/barback/lib/src/asset...
pkg/barback/lib/src/asset_set.dart:59: // TODO(rnystrom): Implement and test
these as needed.
This may be another reason not to implement Set.

[Bob Nystrom, 2013-07-08 17:54:01]

Thanks!

https://codereview.chromium.org/18650004/diff/10001/pkg/barback/lib/src/asset...
File pkg/barback/lib/src/asset_set.dart (right):

https://codereview.chromium.org/18650004/diff/10001/pkg/barback/lib/src/asset...
pkg/barback/lib/src/asset_set.dart:47: }
On 2013/07/03 22:58:33, nweiz wrote:
> If [contains] has a different notion of equality than the rest of the class,
> this shouldn't implement Set.

It does have the same notion of equality. It looks up the existing asset by ID
for performance but it should otherwise behave like a vanilla Set here.

Where the behavior does differ is add(). With an AssetSet, adding a different
Asset with the same ID will replace the previous one with that ID. A normal Set
will keep them both.

But, either way, implementing Set<T> here wasn't buying us anything yet, so I
removed it.

https://codereview.chromium.org/18650004/diff/10001/pkg/barback/lib/src/asset...
pkg/barback/lib/src/asset_set.dart:59: // TODO(rnystrom): Implement and test
these as needed.
On 2013/07/03 22:58:33, nweiz wrote:
> This may be another reason not to implement Set.

Done.

[nweiz, 2013-07-08 19:59:29]

One suggestion, otherwise lgtm.

https://codereview.chromium.org/18650004/diff/25001/pkg/barback/lib/src/asset...
File pkg/barback/lib/src/asset_set.dart (right):

https://codereview.chromium.org/18650004/diff/25001/pkg/barback/lib/src/asset...
pkg/barback/lib/src/asset_set.dart:14: /// A [Set] of [Asset]s with distinct
IDs.
"[Set]" -> "set"

[Bob Nystrom, 2013-07-08 21:20:59]

Committed patchset #6 manually as r24819 (presubmit successful).

[Bob Nystrom, 2013-07-08 21:54:04]

https://codereview.chromium.org/18650004/diff/25001/pkg/barback/lib/src/asset...
File pkg/barback/lib/src/asset_set.dart (right):

https://codereview.chromium.org/18650004/diff/25001/pkg/barback/lib/src/asset...
pkg/barback/lib/src/asset_set.dart:14: /// A [Set] of [Asset]s with distinct
IDs.
On 2013/07/08 19:59:30, nweiz wrote:
> "[Set]" -> "set"

Done.