[dart2dart] Cut declaration types by default if we check that it is safe to do so.

[dart2dart] Cut declaration types by default if we check that it is safe to do so.

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

[Anton Muhin, 2012-09-17 18:45:15]

https://codereview.chromium.org/10917298/diff/1/lib/compiler/implementation/d...
File lib/compiler/implementation/dart_backend/backend.dart (right):

https://codereview.chromium.org/10917298/diff/1/lib/compiler/implementation/d...
lib/compiler/implementation/dart_backend/backend.dart:93: * Tells whether it is
safe to remove type declarations from variables, functions
nit: isn't this string too long?

https://codereview.chromium.org/10917298/diff/1/lib/compiler/implementation/d...
lib/compiler/implementation/dart_backend/backend.dart:101: bool
isSafeToRemoveTypeDeclarations(Map<ClassElement, Set<Element>> classMembers) {
generic question: why do you have to parse elements?  isn't information in
DartTypes enough for you?

https://codereview.chromium.org/10917298/diff/1/lib/compiler/implementation/d...
lib/compiler/implementation/dart_backend/backend.dart:104:
workQueue.addAll(classMembers.getKeys().map((classElement) =>
classElement.type));
ditto for this line and all the lines below

https://codereview.chromium.org/10917298/diff/1/lib/compiler/implementation/d...
lib/compiler/implementation/dart_backend/backend.dart:113: for (Node
typeArgument in typeArguments.nodes.toList()) {
do you need .toList()?

https://codereview.chromium.org/10917298/diff/1/lib/compiler/implementation/d...
lib/compiler/implementation/dart_backend/backend.dart:121:
workQueue.add(argumentType);
shouldn't you check processedTypes before adding anything into the queue?

https://codereview.chromium.org/10917298/diff/1/lib/compiler/implementation/d...
lib/compiler/implementation/dart_backend/backend.dart:353: /*
commented out code

[Roman, 2012-09-18 10:24:08]

https://codereview.chromium.org/10917298/diff/1/lib/compiler/implementation/d...
File lib/compiler/implementation/dart_backend/backend.dart (right):

https://codereview.chromium.org/10917298/diff/1/lib/compiler/implementation/d...
lib/compiler/implementation/dart_backend/backend.dart:93: * Tells whether it is
safe to remove type declarations from variables, functions
On 2012/09/17 18:45:15, Anton Muhin wrote:
> nit: isn't this string too long?

Sorry, forgot to check long lines before sending to review. Fixed everywhere.

https://codereview.chromium.org/10917298/diff/1/lib/compiler/implementation/d...
lib/compiler/implementation/dart_backend/backend.dart:101: bool
isSafeToRemoveTypeDeclarations(Map<ClassElement, Set<Element>> classMembers) {
On 2012/09/17 18:45:15, Anton Muhin wrote:
> generic question: why do you have to parse elements?  isn't information in
> DartTypes enough for you?

Unfortunately, no. When I have:
class A extends B<C> {}

then type A supertype refers to generic B<T> instead of concrete B<C>.

https://codereview.chromium.org/10917298/diff/1/lib/compiler/implementation/d...
lib/compiler/implementation/dart_backend/backend.dart:113: for (Node
typeArgument in typeArguments.nodes.toList()) {
On 2012/09/17 18:45:15, Anton Muhin wrote:
> do you need .toList()?

Indeed I don't, Link is iterable. Removed.

https://codereview.chromium.org/10917298/diff/1/lib/compiler/implementation/d...
lib/compiler/implementation/dart_backend/backend.dart:121:
workQueue.add(argumentType);
On 2012/09/17 18:45:15, Anton Muhin wrote:
> shouldn't you check processedTypes before adding anything into the queue?

Not necessary, when I pop next work element, I check if it was processed.

https://codereview.chromium.org/10917298/diff/1/lib/compiler/implementation/d...
lib/compiler/implementation/dart_backend/backend.dart:353: /*
On 2012/09/17 18:45:15, Anton Muhin wrote:
> commented out code

That was my first emotional attempt to write another visitor./ Removed.

[Anton Muhin, 2012-09-18 10:27:02]

LGTM, but test would be nice to have

https://codereview.chromium.org/10917298/diff/1/lib/compiler/implementation/d...
File lib/compiler/implementation/dart_backend/backend.dart (right):

https://codereview.chromium.org/10917298/diff/1/lib/compiler/implementation/d...
lib/compiler/implementation/dart_backend/backend.dart:121:
workQueue.add(argumentType);
Yes, I know, we can just consume less memory.  Overall, you may want to revisit
your approach and bailout earlier on addition of 'bad' type.

On 2012/09/18 10:24:08, Roman wrote:
> On 2012/09/17 18:45:15, Anton Muhin wrote:
> > shouldn't you check processedTypes before adding anything into the queue?
> 
> Not necessary, when I pop next work element, I check if it was processed.

[Roman, 2012-09-18 13:12:21]

The changes are covered with language tests and unparser tests.

I also made some more changes, to make one test from co19 not fail, see changes
in placeholder_collector

https://codereview.chromium.org/10917298/diff/1/lib/compiler/implementation/d...
File lib/compiler/implementation/dart_backend/backend.dart (right):

https://codereview.chromium.org/10917298/diff/1/lib/compiler/implementation/d...
lib/compiler/implementation/dart_backend/backend.dart:121:
workQueue.add(argumentType);
On 2012/09/18 10:27:03, Anton Muhin wrote:
> Yes, I know, we can just consume less memory.  Overall, you may want to
revisit
> your approach and bailout earlier on addition of 'bad' type.
> 
> On 2012/09/18 10:24:08, Roman wrote:
> > On 2012/09/17 18:45:15, Anton Muhin wrote:
> > > shouldn't you check processedTypes before adding anything into the queue?
> > 
> > Not necessary, when I pop next work element, I check if it was processed.
> 

It is possible that we can add processed items several times, but right now it's
not clear that we're wasting much memory or are experiencing significant
slowdown because of this.

[Anton Muhin, 2012-09-19 06:51:37]

lgtm

https://codereview.chromium.org/10917298/diff/10001/lib/compiler/implementati...
File lib/compiler/implementation/dart_backend/placeholder_collector.dart
(right):

https://codereview.chromium.org/10917298/diff/10001/lib/compiler/implementati...
lib/compiler/implementation/dart_backend/placeholder_collector.dart:520: // Make
sure we don't omit return type of methods which names are
TODO to change that and bug against VM?

[Roman, 2012-09-19 09:17:34]

https://codereview.chromium.org/10917298/diff/10001/lib/compiler/implementati...
File lib/compiler/implementation/dart_backend/placeholder_collector.dart
(right):

https://codereview.chromium.org/10917298/diff/10001/lib/compiler/implementati...
lib/compiler/implementation/dart_backend/placeholder_collector.dart:520: // Make
sure we don't omit return type of methods which names are
On 2012/09/19 06:51:37, Anton Muhin wrote:
> TODO to change that and bug against VM?

http://code.google.com/p/dart/issues/detail?id=5278