Hashcodes were being improperly calculated for public API of libraries.

Hashcodes were being improperly calculated for public API of libraries. 
This triggers reparsing of files dependent on the API.  The error occured
because we were checking hashcodes in the frontend while calculating them
after normalization/backend.  This patch:
-Moves calculation of hashcodes to the same functions that generate the
API files.
-Writes the cached hashcode to depenency files
-Includes an end to end test to guard against regression

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

[codefu, 2011-10-10 22:04:39]

Performance numbers against Total:
full-compile: Before/After-ms(5595 / 5441), stdev(138 / 265), Difference: 154ms
(2.75%),  Performance: No Change
zero-delta-compile: Before/After-ms(779 / 767), stdev(45 / 36), Difference: 12ms
(1.54%),  Performance: No Change
one-delta-compile: Before/After-ms(4639 / 3452), stdev(140 / 260), Difference:
1187ms (25.58%),  Performance: Better

[codefu, 2011-10-10 22:05:22]

added reviewers (sigh)

[jbrosenberg, 2011-10-10 22:46:01]

Looking good, I have some comments.

It looks like we still have the 2 different ways to compute a hash code, one
using the new substring range in DartUnit.toDietSource(), and also the method in
DartNode.computeHash().  Could the version in computeHash use the same
constructor with the ClassPositionMap business (even though it would only have
one class in it).  I think this would make it a little clearer and guard against
introduced leading spaces, comments, etc....

The test coverage is a bit confusing.  It doesn't look like a specific test was
added to test this problem (instead it looks like you added new code in
pre-existing test cases, which makes it less clear what test case is testing
what, etc.).  It's not clear from the additions how the test cases are testing
this fix.

http://codereview.chromium.org/8222016/diff/1/compiler/java/com/google/dart/c...
File compiler/java/com/google/dart/compiler/ast/DartToSourceVisitor.java (left):

http://codereview.chromium.org/8222016/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/ast/DartToSourceVisitor.java:109:
p("function ");
This is fixing a bug in handling the 'function' keyword?

http://codereview.chromium.org/8222016/diff/1/compiler/java/com/google/dart/c...
File compiler/java/com/google/dart/compiler/ast/DartToSourceVisitor.java
(right):

http://codereview.chromium.org/8222016/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/ast/DartToSourceVisitor.java:21: */
An alternate suggestion is to sub-class DartToSourceVisitor, and only override
the method: 
public boolean visit(DartClass x, DartContext ctx)
and have the method do:

start = out.getPosition();
super.visit(x, ctx);
end = out.getPosition();

etc.

http://codereview.chromium.org/8222016/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/ast/DartToSourceVisitor.java:29: public
class ClassPositionMap {
Suggest "ClassPosition"?  "Map" has other connotations, like you can look stuff
up in it with a key, etc.

http://codereview.chromium.org/8222016/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/ast/DartToSourceVisitor.java:53: private
ArrayList<ClassPositionMap> classMap = null;
make final, and initialize either as null or with a value in the constructor
(the new third constructor can be called from the 2nd constructor, etc.).

http://codereview.chromium.org/8222016/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/ast/DartToSourceVisitor.java:159: int
start = out.getPosition();
should this also be conditioned on if (classMap != null)?

http://codereview.chromium.org/8222016/diff/1/compiler/java/com/google/dart/c...
File compiler/java/com/google/dart/compiler/ast/DartUnit.java (right):

http://codereview.chromium.org/8222016/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/ast/DartUnit.java:137:
dietParse.substring(dartClassPosition.getStart(),
dartClassPosition.getEnd()).hashCode());
Suggest pushing the hashCode generation into the ClassPositionMap class, e.g.
just call:]
setHash(dartClassPosition.getHash());

This enforces consistency for other users of this api.

http://codereview.chromium.org/8222016/diff/1/compiler/javatests/com/google/d...
File compiler/javatests/com/google/dart/compiler/end2end/inc/my.dart (right):

http://codereview.chromium.org/8222016/diff/1/compiler/javatests/com/google/d...
compiler/javatests/com/google/dart/compiler/end2end/inc/my.dart:64: // Test
against normalization affecting hashcodes
How does this test that?

http://codereview.chromium.org/8222016/diff/1/compiler/javatests/com/google/d...
File compiler/javatests/com/google/dart/compiler/end2end/inc/some.dart (right):

http://codereview.chromium.org/8222016/diff/1/compiler/javatests/com/google/d...
compiler/javatests/com/google/dart/compiler/end2end/inc/some.dart:13: //
my.dart/Baz deps SomeClass2Impl deps SomeClass2
I'm not sure I understand this comment?  Is it "my.dar/Baz depends on
SomeClass2Impl, which depends on SomeClass2"?

http://codereview.chromium.org/8222016/diff/1/compiler/javatests/com/google/d...
compiler/javatests/com/google/dart/compiler/end2end/inc/some.dart:14: // Test
hashcode against normalization
How does this test that?

[codefu, 2011-10-11 13:56:39]

On 2011/10/10 22:46:01, jbrosenberg wrote:
> It looks like we still have the 2 different ways to compute a hash code, one
> using the new substring range in DartUnit.toDietSource(), and also the method
in
> DartNode.computeHash().  

The DartNode.computeHash() is only handling a DartTypeNode for "String", which
appears as a top level variable in our isolate implementation:
final String _SPAWNED_SIGNAL = "spawned";

Its a different bug as to why this is showing up only for isolate.dart.

> Could the version in computeHash use the same
> constructor with the ClassPositionMap business (even though it would only have
> one class in it).  I think this would make it a little clearer and guard
against
> introduced leading spaces, comments, etc....

I can modify it, but its only running against a single word, and after the
backend is run.

> The test coverage is a bit confusing.  It doesn't look like a specific test
was
> added to test this problem (instead it looks like you added new code in
> pre-existing test cases, which makes it less clear what test case is testing
> what, etc.).  It's not clear from the additions how the test cases are testing
> this fix.

testTouchOneSource() tests for the recompilation.  The bug that entered after
normalization was not being tested for in this case, so no test was needed to be
modified directly, only the source for the test slightly adjusted.

[codefu, 2011-10-11 14:18:07]

http://codereview.chromium.org/8222016/diff/1/compiler/java/com/google/dart/c...
File compiler/java/com/google/dart/compiler/ast/DartToSourceVisitor.java (left):

http://codereview.chromium.org/8222016/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/ast/DartToSourceVisitor.java:109:
p("function ");
On 2011/10/10 22:46:01, jbrosenberg wrote:
> This is fixing a bug in handling the 'function' keyword?

There is no "function" keyword, it was removed a while ago.  The effect of this
is to now inject a previously non-existent identifier if there was no return
variable, which is not necessary.

http://codereview.chromium.org/8222016/diff/1/compiler/java/com/google/dart/c...
File compiler/java/com/google/dart/compiler/ast/DartToSourceVisitor.java
(right):

http://codereview.chromium.org/8222016/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/ast/DartToSourceVisitor.java:29: public
class ClassPositionMap {
On 2011/10/10 22:46:01, jbrosenberg wrote:
> Suggest "ClassPosition"?  "Map" has other connotations, like you can look
stuff
> up in it with a key, etc.

Done.

http://codereview.chromium.org/8222016/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/ast/DartToSourceVisitor.java:53: private
ArrayList<ClassPositionMap> classMap = null;
On 2011/10/10 22:46:01, jbrosenberg wrote:
> make final, and initialize either as null or with a value in the constructor
> (the new third constructor can be called from the 2nd constructor, etc.).

Done.

http://codereview.chromium.org/8222016/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/ast/DartToSourceVisitor.java:159: int
start = out.getPosition();
On 2011/10/10 22:46:01, jbrosenberg wrote:
> should this also be conditioned on if (classMap != null)?

I thought about doing that, but I thought the code would look odd:
int start = 0;
if (classMap != null) {
  start = out.getPosition();
}

I can update it if you like.

http://codereview.chromium.org/8222016/diff/1/compiler/java/com/google/dart/c...
File compiler/java/com/google/dart/compiler/ast/DartUnit.java (right):

http://codereview.chromium.org/8222016/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/ast/DartUnit.java:137:
dietParse.substring(dartClassPosition.getStart(),
dartClassPosition.getEnd()).hashCode());
On 2011/10/10 22:46:01, jbrosenberg wrote:
> Suggest pushing the hashCode generation into the ClassPositionMap class, e.g.
> just call:]
> setHash(dartClassPosition.getHash());
> 
> This enforces consistency for other users of this api.

Better yet, remove ClassPosition and just initialize DTS with "boolean
calculateHash".

http://codereview.chromium.org/8222016/diff/1/compiler/javatests/com/google/d...
File compiler/javatests/com/google/dart/compiler/end2end/inc/my.dart (right):

http://codereview.chromium.org/8222016/diff/1/compiler/javatests/com/google/d...
compiler/javatests/com/google/dart/compiler/end2end/inc/my.dart:64: // Test
against normalization affecting hashcodes
On 2011/10/10 22:46:01, jbrosenberg wrote:
> How does this test that?

SomeClass2 is the interface that gets the default constructor injected by the
dartc backend.  Just implementing that interface here would only cause a hash
inconsistency in this file and we'd only compile this one.  By depending on a
class that implements the interface in a different file, we'll see that class
fail its dependency check and two files be recompiled every time one file is
touched.

http://codereview.chromium.org/8222016/diff/1/compiler/javatests/com/google/d...
File compiler/javatests/com/google/dart/compiler/end2end/inc/some.dart (right):

http://codereview.chromium.org/8222016/diff/1/compiler/javatests/com/google/d...
compiler/javatests/com/google/dart/compiler/end2end/inc/some.dart:13: //
my.dart/Baz deps SomeClass2Impl deps SomeClass2
On 2011/10/10 22:46:01, jbrosenberg wrote:
> I'm not sure I understand this comment?  Is it "my.dar/Baz depends on
> SomeClass2Impl, which depends on SomeClass2"?

Yes.

http://codereview.chromium.org/8222016/diff/1/compiler/javatests/com/google/d...
compiler/javatests/com/google/dart/compiler/end2end/inc/some.dart:14: // Test
hashcode against normalization
On 2011/10/10 22:46:01, jbrosenberg wrote:
> How does this test that?

previously commented; will add a more detailed comment here.

[jbrosenberg, 2011-10-11 14:58:44]

I think things are cleaner now in DartToSourceVisitor.

I still think the alternate hash calculation in DartNode.computeHash() should
use the same api (e.g. pass 'true' for the computeHash arg to the
DartToSourceVisitor constructor, and then not call hashCode() externally to
DartToSourceVisitor.

I'm still confused about the test coverage. Would it be simpler to just add a
new test case to IncrementalCompileTest, which reproduces the error that would
exist prior to this fix, and add the explanation there in the test case.  Since
the various my.*.dart files are used by multiple test cases, probably don't need
quite so much specific explanation there?

http://codereview.chromium.org/8222016/diff/1/compiler/java/com/google/dart/c...
File compiler/java/com/google/dart/compiler/ast/DartToSourceVisitor.java
(right):

http://codereview.chromium.org/8222016/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/ast/DartToSourceVisitor.java:159: int
start = out.getPosition();
This is an example of something that might be cleaner if you have a sub-class
(e.g. DartToSourceVisitorWithClassMap extends DartToSourceVisitor)

http://codereview.chromium.org/8222016/diff/8001/compiler/java/com/google/dar...
File compiler/java/com/google/dart/compiler/ast/DartToSourceVisitor.java
(right):

http://codereview.chromium.org/8222016/diff/8001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/ast/DartToSourceVisitor.java:29: private
final boolean calculateHash;
Much cleaner!

http://codereview.chromium.org/8222016/diff/8001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/ast/DartToSourceVisitor.java:189: if
(calculateHash == true) {
Should this appear after the 2 nl(); calls?  Just thinking it makes sense to
have it at the end of everything (since yo u have other calls to nl() prior to
this, etc.)

http://codereview.chromium.org/8222016/diff/8001/compiler/javatests/com/googl...
File compiler/javatests/com/google/dart/compiler/end2end/inc/my.dart (right):

http://codereview.chromium.org/8222016/diff/8001/compiler/javatests/com/googl...
compiler/javatests/com/google/dart/compiler/end2end/inc/my.dart:66: // by the
dartc backend.  Just implementing that interface here would only
I'm not sure I understand this (what do you mean by it's "the default
constructor injected..."?  How does the act of implementing the interface cause
an inconsistency?

http://codereview.chromium.org/8222016/diff/8001/compiler/javatests/com/googl...
compiler/javatests/com/google/dart/compiler/end2end/inc/my.dart:69: // file
(SomeClass2), we'll see that class fail its hash check and two files be
Will we still see this fail, with this fix?

http://codereview.chromium.org/8222016/diff/8001/compiler/javatests/com/googl...
File compiler/javatests/com/google/dart/compiler/end2end/inc/my.no5ref.dart
(right):

http://codereview.chromium.org/8222016/diff/8001/compiler/javatests/com/googl...
compiler/javatests/com/google/dart/compiler/end2end/inc/my.no5ref.dart:52: //
Test against normalization affecting hashcodes
I'm not sure how this tests against normalization affecting things?

[codefu, 2011-10-11 18:43:54]

Moved Baz to myother7.dart and created a new test case, even though 13 other
tests will also fail.

PTAL

http://codereview.chromium.org/8222016/diff/8001/compiler/java/com/google/dar...
File compiler/java/com/google/dart/compiler/ast/DartToSourceVisitor.java
(right):

http://codereview.chromium.org/8222016/diff/8001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/ast/DartToSourceVisitor.java:189: if
(calculateHash == true) {
On 2011/10/11 14:58:44, jbrosenberg wrote:
> Should this appear after the 2 nl(); calls?  Just thinking it makes sense to
> have it at the end of everything (since yo u have other calls to nl() prior to
> this, etc.)

This emulates the work of older code that called trim() before calculating the
hash.  I don't feel that whitespace after the class' "}" adds any value.

http://codereview.chromium.org/8222016/diff/8001/compiler/javatests/com/googl...
File compiler/javatests/com/google/dart/compiler/end2end/inc/my.dart (right):

http://codereview.chromium.org/8222016/diff/8001/compiler/javatests/com/googl...
compiler/javatests/com/google/dart/compiler/end2end/inc/my.dart:66: // by the
dartc backend.  Just implementing that interface here would only
On 2011/10/11 14:58:44, jbrosenberg wrote:
> I'm not sure I understand this (what do you mean by it's "the default
> constructor injected..."?  How does the act of implementing the interface
cause
> an inconsistency?

normalization adds "(){}" to the interface, corrupting the dep's hashcode which
will not match a hashcode of the diet API.

http://codereview.chromium.org/8222016/diff/8001/compiler/javatests/com/googl...
compiler/javatests/com/google/dart/compiler/end2end/inc/my.dart:69: // file
(SomeClass2), we'll see that class fail its hash check and two files be
On 2011/10/11 14:58:44, jbrosenberg wrote:
> Will we still see this fail, with this fix?

good catch.

http://codereview.chromium.org/8222016/diff/8001/compiler/javatests/com/googl...
File compiler/javatests/com/google/dart/compiler/end2end/inc/my.no5ref.dart
(right):

http://codereview.chromium.org/8222016/diff/8001/compiler/javatests/com/googl...
compiler/javatests/com/google/dart/compiler/end2end/inc/my.no5ref.dart:52: //
Test against normalization affecting hashcodes
On 2011/10/11 14:58:44, jbrosenberg wrote:
> I'm not sure how this tests against normalization affecting things?

Recap of conversation:
Interface gets default constructor "(){}" added at normalization. Anything
depending on that interface will have a hashcode mismatch between that libraries
'.deps' file and the API of the interface's library.  Just implementing the
interface here would only cause this file to recompile.  Implementing the
interface in another lib and depending on it will cause both files to re-parse
and re-compile when any file in 'my.app.dart' is touched.

[jbrosenberg, 2011-10-11 21:40:13]

Very close.

Can we still have a single class that processes the hashCode generation?  (or we
can leave as a TODO and address later as part of a larger cleanup).

http://codereview.chromium.org/8222016/diff/8001/compiler/java/com/google/dar...
File compiler/java/com/google/dart/compiler/ast/DartToSourceVisitor.java
(right):

http://codereview.chromium.org/8222016/diff/8001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/ast/DartToSourceVisitor.java:189: if
(calculateHash == true) {
But I think we still have calls coming into this method, which don't pass the
'calculateHash' flag, and rely on getting the output returned for a whole node. 
Thus, if this whitespace could affect the hash, it should be included in the
calculation.

http://codereview.chromium.org/8222016/diff/7003/compiler/java/com/google/dar...
File compiler/java/com/google/dart/compiler/ast/DartToSourceVisitor.java
(right):

http://codereview.chromium.org/8222016/diff/7003/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/ast/DartToSourceVisitor.java:189: if
(calculateHash == true) {
But I think we still have calls coming into this method, which don't pass the
'calculateHash' flag, and rely on getting the output returned for a whole node. 
Thus, if this whitespace could affect the hash, it should be included in the
calculation.

http://codereview.chromium.org/8222016/diff/7003/compiler/javatests/com/googl...
File
compiler/javatests/com/google/dart/compiler/end2end/inc/IncrementalCompilationTest.java
(right):

http://codereview.chromium.org/8222016/diff/7003/compiler/javatests/com/googl...
compiler/javatests/com/google/dart/compiler/end2end/inc/IncrementalCompilationTest.java:185:
// SomeInterface2 is and empty interface that has a default constructor added
s/is and/is an/
s/has a default constructor added by the dartc backend/can have an extra default
constructor added by the dartc backend normalization process.

http://codereview.chromium.org/8222016/diff/7003/compiler/javatests/com/googl...
compiler/javatests/com/google/dart/compiler/end2end/inc/IncrementalCompilationTest.java:186:
// by the dartc backend.  By depending on a class that implments the interface,
s/implments/implements

http://codereview.chromium.org/8222016/diff/7003/compiler/javatests/com/googl...
compiler/javatests/com/google/dart/compiler/end2end/inc/IncrementalCompilationTest.java:187:
// my.app.dart.deps will transitivy depend on the empty interface.  If something
changes
s/transitivy/transitively

http://codereview.chromium.org/8222016/diff/7003/compiler/javatests/com/googl...
compiler/javatests/com/google/dart/compiler/end2end/inc/IncrementalCompilationTest.java:188:
// with how hashcodes are generated, this test should fail.
s/this test should fail/this test should be updated

http://codereview.chromium.org/8222016/diff/7003/compiler/javatests/com/googl...
compiler/javatests/com/google/dart/compiler/end2end/inc/IncrementalCompilationTest.java:191:
// be changed. At present, the app's deps and api will be rewritten.
Is it possible in the future that deps/api won't be rewritten?

http://codereview.chromium.org/8222016/diff/7003/compiler/javatests/com/googl...
compiler/javatests/com/google/dart/compiler/end2end/inc/IncrementalCompilationTest.java:198:
// Nothing else should have changed.
maybe also have a check here for:
didNotWrite("my.dart");

http://codereview.chromium.org/8222016/diff/7003/compiler/javatests/com/googl...
File compiler/javatests/com/google/dart/compiler/end2end/inc/my.no5ref.dart
(left):

http://codereview.chromium.org/8222016/diff/7003/compiler/javatests/com/googl...
compiler/javatests/com/google/dart/compiler/end2end/inc/my.no5ref.dart:50: }
Did you mean to remove this?

http://codereview.chromium.org/8222016/diff/7003/compiler/javatests/com/googl...
File compiler/javatests/com/google/dart/compiler/end2end/inc/some.dart (right):

http://codereview.chromium.org/8222016/diff/7003/compiler/javatests/com/googl...
compiler/javatests/com/google/dart/compiler/end2end/inc/some.dart:13: //
myother7.dart/Baz depends on SomeClass2 depends on SomeInterface2
s/SomeClass2 depends on/SomeClass2 which depends on
(and same in a couple other files with the same comment)

[codefu, 2011-10-11 22:07:18]

http://codereview.chromium.org/8222016/diff/8001/compiler/java/com/google/dar...
File compiler/java/com/google/dart/compiler/ast/DartToSourceVisitor.java
(right):

http://codereview.chromium.org/8222016/diff/8001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/ast/DartToSourceVisitor.java:189: if
(calculateHash == true) {
On 2011/10/11 21:40:13, jbrosenberg wrote:
> But I think we still have calls coming into this method, which don't pass the
> 'calculateHash' flag, and rely on getting the output returned for a whole
node. 
> Thus, if this whitespace could affect the hash, it should be included in the
> calculation.

DartNode.java:
  return out.toString().trim().hashCode();

http://codereview.chromium.org/8222016/diff/7003/compiler/java/com/google/dar...
File compiler/java/com/google/dart/compiler/ast/DartToSourceVisitor.java
(right):

http://codereview.chromium.org/8222016/diff/7003/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/ast/DartToSourceVisitor.java:189: if
(calculateHash == true) {
On 2011/10/11 21:40:13, jbrosenberg wrote:
> But I think we still have calls coming into this method, which don't pass the
> 'calculateHash' flag, and rely on getting the output returned for a whole
node. 
> Thus, if this whitespace could affect the hash, it should be included in the
> calculation.

dito on previous DartNode + trim()

http://codereview.chromium.org/8222016/diff/7003/compiler/javatests/com/googl...
File
compiler/javatests/com/google/dart/compiler/end2end/inc/IncrementalCompilationTest.java
(right):

http://codereview.chromium.org/8222016/diff/7003/compiler/javatests/com/googl...
compiler/javatests/com/google/dart/compiler/end2end/inc/IncrementalCompilationTest.java:185:
// SomeInterface2 is and empty interface that has a default constructor added
On 2011/10/11 21:40:13, jbrosenberg wrote:
> s/is and/is an/
> s/has a default constructor added by the dartc backend/can have an extra
default
> constructor added by the dartc backend normalization process.

Done.

http://codereview.chromium.org/8222016/diff/7003/compiler/javatests/com/googl...
compiler/javatests/com/google/dart/compiler/end2end/inc/IncrementalCompilationTest.java:186:
// by the dartc backend.  By depending on a class that implments the interface,
On 2011/10/11 21:40:13, jbrosenberg wrote:
> s/implments/implements

Done.

http://codereview.chromium.org/8222016/diff/7003/compiler/javatests/com/googl...
compiler/javatests/com/google/dart/compiler/end2end/inc/IncrementalCompilationTest.java:187:
// my.app.dart.deps will transitivy depend on the empty interface.  If something
changes
On 2011/10/11 21:40:13, jbrosenberg wrote:
> s/transitivy/transitively

Done.

http://codereview.chromium.org/8222016/diff/7003/compiler/javatests/com/googl...
compiler/javatests/com/google/dart/compiler/end2end/inc/IncrementalCompilationTest.java:188:
// with how hashcodes are generated, this test should fail.
On 2011/10/11 21:40:13, jbrosenberg wrote:
> s/this test should fail/this test should be updated

Done.

http://codereview.chromium.org/8222016/diff/7003/compiler/javatests/com/googl...
compiler/javatests/com/google/dart/compiler/end2end/inc/IncrementalCompilationTest.java:191:
// be changed. At present, the app's deps and api will be rewritten.
On 2011/10/11 21:40:13, jbrosenberg wrote:
> Is it possible in the future that deps/api won't be rewritten?

Perhaps.  Previous comments include "This might be optimized later."

http://codereview.chromium.org/8222016/diff/7003/compiler/javatests/com/googl...
compiler/javatests/com/google/dart/compiler/end2end/inc/IncrementalCompilationTest.java:198:
// Nothing else should have changed.
On 2011/10/11 21:40:13, jbrosenberg wrote:
> maybe also have a check here for:
> didNotWrite("my.dart");

Done.

http://codereview.chromium.org/8222016/diff/7003/compiler/javatests/com/googl...
File compiler/javatests/com/google/dart/compiler/end2end/inc/my.no5ref.dart
(left):

http://codereview.chromium.org/8222016/diff/7003/compiler/javatests/com/googl...
compiler/javatests/com/google/dart/compiler/end2end/inc/my.no5ref.dart:50: }
On 2011/10/11 21:40:13, jbrosenberg wrote:
> Did you mean to remove this?

Overzealous cut.

http://codereview.chromium.org/8222016/diff/7003/compiler/javatests/com/googl...
File compiler/javatests/com/google/dart/compiler/end2end/inc/some.dart (right):

http://codereview.chromium.org/8222016/diff/7003/compiler/javatests/com/googl...
compiler/javatests/com/google/dart/compiler/end2end/inc/some.dart:13: //
myother7.dart/Baz depends on SomeClass2 depends on SomeInterface2
On 2011/10/11 21:40:13, jbrosenberg wrote:
> s/SomeClass2 depends on/SomeClass2 which depends on
> (and same in a couple other files with the same comment)

Done.

[jbrosenberg, 2011-10-11 22:17:57]

LGTM