Retain elements a finer granularity than library.

Retain elements a finer granularity than library.

BUG=http://dartbug.com/11524
R=johnniwinther@google.com, nweiz@google.com

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

[ahe, 2013-08-05 15:14:52]

Hi Luke,

Could you take a look at the file called "warnings.dart" and see if the new
guide lines make sense. Also, I've added a bunch of new diagnostics that follow
the new guide lines. I'd appreciate your thoughts on those.

Cheers,
Peter

[ahe, 2013-08-05 15:23:15]

Hi Nathan,

Could you take a look at my changes to browser_test.dart?

Cheers,
Peter

[kevmoo-old, 2013-08-05 19:29:17]

12125 was merged into https://code.google.com/p/dart/issues/detail?id=11524

[nweiz, 2013-08-05 19:55:42]

browser_test change lgtm

[ahe, 2013-08-05 20:07:08]

On 2013/08/05 19:29:17, kevmoo wrote:
> 12125 was merged into https://code.google.com/p/dart/issues/detail?id=11524

Thanks for spotting that. I've updated the link.

[Johnni Winther, 2013-08-07 10:39:17]

Following your own standards this CL should have been split into 2-4 minor CLs.
But I'm not you: LGTM

https://codereview.chromium.org/21242002/diff/6001/dart/sdk/lib/_internal/com...
File dart/sdk/lib/_internal/compiler/implementation/mirrors_used.dart (right):

https://codereview.chromium.org/21242002/diff/6001/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/mirrors_used.dart:5: library
dart2js.mirrors_used;
Overall for this file: Document methods.

https://codereview.chromium.org/21242002/diff/6001/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/mirrors_used.dart:406: DartType
type = constant.computeType(compiler);
This doesn't work for double constants with zero fraction part, since our JS
constant system turns these into int-constants.

https://codereview.chromium.org/21242002/diff/6001/dart/sdk/lib/_internal/com...
File dart/sdk/lib/_internal/compiler/implementation/warnings.dart (right):

https://codereview.chromium.org/21242002/diff/6001/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/warnings.dart:716: howToFix: "Did
you check the spelling of '#{name}'?",
Strange question to ask (they probably _forgot_ to check the spelling, if
anything). Perhaps 'Did you misspell '#name'?

https://codereview.chromium.org/21242002/diff/6001/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/warnings.dart:729: howToFix: "Did
you check the spelling of '#{name}'?",
Ditto.

https://codereview.chromium.org/21242002/diff/6001/dart/tests/compiler/dart2j...
File dart/tests/compiler/dart2js/message_kind_helper.dart (right):

https://codereview.chromium.org/21242002/diff/6001/dart/tests/compiler/dart2j...
dart/tests/compiler/dart2js/message_kind_helper.dart:45: Expect.isTrue(new
RegExp('^$pattern\$').hasMatch(message), '"$pattern" does not match
"$message"');
Long line.

[karlklose, 2013-08-07 10:48:11]

DBC:

https://codereview.chromium.org/21242002/diff/6001/dart/sdk/lib/_internal/com...
File dart/sdk/lib/_internal/compiler/implementation/warnings.dart (right):

https://codereview.chromium.org/21242002/diff/6001/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/warnings.dart:716: howToFix: "Did
you check the spelling of '#{name}'?",
It would be much more useful if we could find the next (two) closest identifiers
in edit-distance and ask "Did you mean 'hest'?", for example.

[ahe, 2013-08-07 11:02:26]

On 2013/08/07 10:39:17, Johnni Winther wrote:
> Following your own standards this CL should have been split into 2-4 minor
CLs.
> But I'm not you: LGTM

I'm me and I thought a lot about this, but I kept convincing myself that I
needed some examples along with the new guidelines. 

However, now that I have to explain why I didn't split it up, I see tons of ways
to split it, and I'm really tempted to do that. It really is easier for me to
deal with small CLs, just to help make sure that everything is tested.

[ahe, 2013-08-07 13:40:20]

Thank you, Johnni and Karl!

https://codereview.chromium.org/21242002/diff/6001/dart/sdk/lib/_internal/com...
File dart/sdk/lib/_internal/compiler/implementation/mirrors_used.dart (right):

https://codereview.chromium.org/21242002/diff/6001/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/mirrors_used.dart:5: library
dart2js.mirrors_used;
On 2013/08/07 10:39:17, Johnni Winther wrote:
> Overall for this file: Document methods.

Done.

https://codereview.chromium.org/21242002/diff/6001/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/mirrors_used.dart:406: DartType
type = constant.computeType(compiler);
On 2013/08/07 10:39:17, Johnni Winther wrote:
> This doesn't work for double constants with zero fraction part, since our JS
> constant system turns these into int-constants.

That sounds like a bug in our constant system.

https://codereview.chromium.org/21242002/diff/6001/dart/sdk/lib/_internal/com...
File dart/sdk/lib/_internal/compiler/implementation/warnings.dart (right):

https://codereview.chromium.org/21242002/diff/6001/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/warnings.dart:716: howToFix: "Did
you check the spelling of '#{name}'?",
On 2013/08/07 10:48:11, karlklose wrote:
> It would be much more useful if we could find the next (two) closest
identifiers
> in edit-distance and ask "Did you mean 'hest'?", for example.

I agree with Karl, but I don't have the next closest identifier. Changed it to:

      // TODO(ahe): The closest identifiers in edit distance would be nice.
      howToFix: "Is '#{name}' spelled right?",

https://codereview.chromium.org/21242002/diff/6001/dart/tests/compiler/dart2j...
File dart/tests/compiler/dart2js/message_kind_helper.dart (right):

https://codereview.chromium.org/21242002/diff/6001/dart/tests/compiler/dart2j...
dart/tests/compiler/dart2js/message_kind_helper.dart:45: Expect.isTrue(new
RegExp('^$pattern\$').hasMatch(message), '"$pattern" does not match
"$message"');
On 2013/08/07 10:39:17, Johnni Winther wrote:
> Long line.

Done.

[ahe, 2013-08-07 13:41:08]

Committed patchset #5 manually as r25873 (presubmit successful).