[dart2dart] Optionally cut types in variable declarations:

[dart2dart] Optionally cut types in variable declarations:
top-level fields, function types, local variable declarations, function parameters.

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

[Anton Muhin, 2012-08-28 09:13:37]

https://chromiumcodereview.appspot.com/10891004/diff/1/lib/compiler/implement...
File lib/compiler/implementation/dart_backend/backend.dart (right):

https://chromiumcodereview.appspot.com/10891004/diff/1/lib/compiler/implement...
lib/compiler/implementation/dart_backend/backend.dart:105:
renamePlaceholders(compiler, collector, renames, imports, minify, false);
please, make it a field, default to false

https://chromiumcodereview.appspot.com/10891004/diff/1/lib/compiler/implement...
File lib/compiler/implementation/dart_backend/placeholder_collector.dart
(right):

https://chromiumcodereview.appspot.com/10891004/diff/1/lib/compiler/implement...
lib/compiler/implementation/dart_backend/placeholder_collector.dart:27: final
bool canOmitType;
canOmitType is somewhat misnomer as in the case of final or const, one must omit
var decl, care provide some other names, something lik requiresVar?

https://chromiumcodereview.appspot.com/10891004/diff/1/lib/compiler/implement...
lib/compiler/implementation/dart_backend/placeholder_collector.dart:229: void
makeDeclarationTypePlaceholder(TypeAnnotation type, bool canOmitType) {
it looks like the only place where canOmitType is true is
makeVarDeclarationPlaceholder.  Given that most probably node.type in this case
cannot be null, can you drop this argument and inline necessary logici nto
makeVDTP?

https://chromiumcodereview.appspot.com/10891004/diff/1/lib/compiler/implement...
lib/compiler/implementation/dart_backend/placeholder_collector.dart:421: for
(Link<Node> link = parameters.nodes; !link.isEmpty(); link= link.tail) {
for (Node parameter in parameters) ?

https://chromiumcodereview.appspot.com/10891004/diff/1/lib/compiler/implement...
lib/compiler/implementation/dart_backend/placeholder_collector.dart:421: for
(Link<Node> link = parameters.nodes; !link.isEmpty(); link= link.tail) {
nit: space before =

https://chromiumcodereview.appspot.com/10891004/diff/1/lib/compiler/implement...
lib/compiler/implementation/dart_backend/placeholder_collector.dart:429:
(parameter as VariableDefinitions).type, true);
parameter.asVariableDefinitions() ?

https://chromiumcodereview.appspot.com/10891004/diff/1/lib/compiler/implement...
lib/compiler/implementation/dart_backend/placeholder_collector.dart:484:
node.statements.nodes.forEach((Node statement) {
plain for loop?

https://chromiumcodereview.appspot.com/10891004/diff/1/lib/compiler/implement...
lib/compiler/implementation/dart_backend/placeholder_collector.dart:486:
makeVarDeclarationTypePlaceholder(statement);
why this cannot be a part of visitVariableDefinitions?

[Anton Muhin, 2012-08-28 09:13:45]

And tests, please

[Roman, 2012-08-28 12:49:30]

https://chromiumcodereview.appspot.com/10891004/diff/1/lib/compiler/implement...
File lib/compiler/implementation/dart_backend/backend.dart (right):

https://chromiumcodereview.appspot.com/10891004/diff/1/lib/compiler/implement...
lib/compiler/implementation/dart_backend/backend.dart:105:
renamePlaceholders(compiler, collector, renames, imports, minify, false);
On 2012/08/28 09:13:37, Anton Muhin wrote:
> please, make it a field, default to false

Done.

https://chromiumcodereview.appspot.com/10891004/diff/1/lib/compiler/implement...
File lib/compiler/implementation/dart_backend/placeholder_collector.dart
(right):

https://chromiumcodereview.appspot.com/10891004/diff/1/lib/compiler/implement...
lib/compiler/implementation/dart_backend/placeholder_collector.dart:27: final
bool canOmitType;
On 2012/08/28 09:13:37, Anton Muhin wrote:
> canOmitType is somewhat misnomer as in the case of final or const, one must
omit
> var decl, care provide some other names, something lik requiresVar?

Done.

https://chromiumcodereview.appspot.com/10891004/diff/1/lib/compiler/implement...
lib/compiler/implementation/dart_backend/placeholder_collector.dart:229: void
makeDeclarationTypePlaceholder(TypeAnnotation type, bool canOmitType) {
On 2012/08/28 09:13:37, Anton Muhin wrote:
> it looks like the only place where canOmitType is true is
> makeVarDeclarationPlaceholder.  Given that most probably node.type in this
case
> cannot be null, can you drop this argument and inline necessary logici nto
> makeVDTP?

I can, but I don't see any benefits. I will save one argument, but will make
code duplication in makeVarDeclarationTypePlaceholder, also method
makeDeclarationTypePlaceholder would hide that the meaning is that I can omit
type, I will need to rename it.

https://chromiumcodereview.appspot.com/10891004/diff/1/lib/compiler/implement...
lib/compiler/implementation/dart_backend/placeholder_collector.dart:421: for
(Link<Node> link = parameters.nodes; !link.isEmpty(); link= link.tail) {
On 2012/08/28 09:13:37, Anton Muhin wrote:
> for (Node parameter in parameters) ?

Can't do this, the structure is: first several Node-s are VariableDefinitions
representing required parameters, then there's a NodeList that has
VariableDefinitions representing optional parameters.
Another option is to do something like this:

for(Node node in parameters.nodes) {
  if (node is NodeList) {
    // Optional parameters.
    collectFunctionParameters(node);
  } else {
    // Required parameter.
    makeDeclarationTypePlaceholder...
  }
}

https://chromiumcodereview.appspot.com/10891004/diff/1/lib/compiler/implement...
lib/compiler/implementation/dart_backend/placeholder_collector.dart:421: for
(Link<Node> link = parameters.nodes; !link.isEmpty(); link= link.tail) {
On 2012/08/28 09:13:37, Anton Muhin wrote:
> nit: space before =

Done.

https://chromiumcodereview.appspot.com/10891004/diff/1/lib/compiler/implement...
lib/compiler/implementation/dart_backend/placeholder_collector.dart:429:
(parameter as VariableDefinitions).type, true);
On 2012/08/28 09:13:37, Anton Muhin wrote:
> parameter.asVariableDefinitions() ?

Done.

https://chromiumcodereview.appspot.com/10891004/diff/1/lib/compiler/implement...
lib/compiler/implementation/dart_backend/placeholder_collector.dart:484:
node.statements.nodes.forEach((Node statement) {
On 2012/08/28 09:13:37, Anton Muhin wrote:
> plain for loop?

Done.

https://chromiumcodereview.appspot.com/10891004/diff/1/lib/compiler/implement...
lib/compiler/implementation/dart_backend/placeholder_collector.dart:486:
makeVarDeclarationTypePlaceholder(statement);
On 2012/08/28 09:13:37, Anton Muhin wrote:
> why this cannot be a part of visitVariableDefinitions?

As discussed, there's a catch statement that I want to skip.
Also, in visitVariableDefinitions I must decide whether declaration requires any
type or 'var', I will have problems if variable definitions node is not resolved
in case of closure function parameters.
Added a todo to maybe move this to visitVariableDefinitions if resolver becomes
better and/or catch syntax changes.

[Anton Muhin, 2012-08-28 13:06:32]

https://chromiumcodereview.appspot.com/10891004/diff/1/lib/compiler/implement...
File lib/compiler/implementation/dart_backend/placeholder_collector.dart
(right):

https://chromiumcodereview.appspot.com/10891004/diff/1/lib/compiler/implement...
lib/compiler/implementation/dart_backend/placeholder_collector.dart:229: void
makeDeclarationTypePlaceholder(TypeAnnotation type, bool canOmitType) {
That's not the matter of saving argument, but the fact that in all other places
types can be omitted which is now immediately seen.

On 2012/08/28 12:49:30, Roman wrote:
> On 2012/08/28 09:13:37, Anton Muhin wrote:
> > it looks like the only place where canOmitType is true is
> > makeVarDeclarationPlaceholder.  Given that most probably node.type in this
> case
> > cannot be null, can you drop this argument and inline necessary logici nto
> > makeVDTP?
> 
> I can, but I don't see any benefits. I will save one argument, but will make
> code duplication in makeVarDeclarationTypePlaceholder, also method
> makeDeclarationTypePlaceholder would hide that the meaning is that I can omit
> type, I will need to rename it.

https://chromiumcodereview.appspot.com/10891004/diff/1/lib/compiler/implement...
lib/compiler/implementation/dart_backend/placeholder_collector.dart:421: for
(Link<Node> link = parameters.nodes; !link.isEmpty(); link= link.tail) {
Yes, this looks more readable to me.

On 2012/08/28 12:49:30, Roman wrote:
> On 2012/08/28 09:13:37, Anton Muhin wrote:
> > for (Node parameter in parameters) ?
> 
> Can't do this, the structure is: first several Node-s are VariableDefinitions
> representing required parameters, then there's a NodeList that has
> VariableDefinitions representing optional parameters.
> Another option is to do something like this:
> 
> for(Node node in parameters.nodes) {
>   if (node is NodeList) {
>     // Optional parameters.
>     collectFunctionParameters(node);
>   } else {
>     // Required parameter.
>     makeDeclarationTypePlaceholder...
>   }
> }

https://chromiumcodereview.appspot.com/10891004/diff/10/tests/compiler/dart2j...
File tests/compiler/dart2js/unparser_test.dart (right):

https://chromiumcodereview.appspot.com/10891004/diff/10/tests/compiler/dart2j...
tests/compiler/dart2js/unparser_test.dart:785: testDeclarationTypePlaceholders()
{
why some complicated?  why not pass a flag to omit type and check against
expected output?

[Roman, 2012-08-28 13:27:51]

https://chromiumcodereview.appspot.com/10891004/diff/1/lib/compiler/implement...
File lib/compiler/implementation/dart_backend/placeholder_collector.dart
(right):

https://chromiumcodereview.appspot.com/10891004/diff/1/lib/compiler/implement...
lib/compiler/implementation/dart_backend/placeholder_collector.dart:229: void
makeDeclarationTypePlaceholder(TypeAnnotation type, bool canOmitType) {
On 2012/08/28 13:06:32, Anton Muhin wrote:
> That's not the matter of saving argument, but the fact that in all other
places
> types can be omitted which is now immediately seen.
> 
> On 2012/08/28 12:49:30, Roman wrote:
> > On 2012/08/28 09:13:37, Anton Muhin wrote:
> > > it looks like the only place where canOmitType is true is
> > > makeVarDeclarationPlaceholder.  Given that most probably node.type in this
> > case
> > > cannot be null, can you drop this argument and inline necessary logici nto
> > > makeVDTP?
> > 
> > I can, but I don't see any benefits. I will save one argument, but will make
> > code duplication in makeVarDeclarationTypePlaceholder, also method
> > makeDeclarationTypePlaceholder would hide that the meaning is that I can
omit
> > type, I will need to rename it.
> 

What about renaming this method to
makeOmitDeclarationTypePlaceholder(TypeAnnotation type) ? I want either this
method name to clearly tell that type can be omitted or have a parameter for
omitting.

https://chromiumcodereview.appspot.com/10891004/diff/1/lib/compiler/implement...
lib/compiler/implementation/dart_backend/placeholder_collector.dart:421: for
(Link<Node> link = parameters.nodes; !link.isEmpty(); link= link.tail) {
On 2012/08/28 13:06:32, Anton Muhin wrote:
> Yes, this looks more readable to me.
> 
> On 2012/08/28 12:49:30, Roman wrote:
> > On 2012/08/28 09:13:37, Anton Muhin wrote:
> > > for (Node parameter in parameters) ?
> > 
> > Can't do this, the structure is: first several Node-s are
VariableDefinitions
> > representing required parameters, then there's a NodeList that has
> > VariableDefinitions representing optional parameters.
> > Another option is to do something like this:
> > 
> > for(Node node in parameters.nodes) {
> >   if (node is NodeList) {
> >     // Optional parameters.
> >     collectFunctionParameters(node);
> >   } else {
> >     // Required parameter.
> >     makeDeclarationTypePlaceholder...
> >   }
> > }
> 

Sorry, which one looks more readable?

https://chromiumcodereview.appspot.com/10891004/diff/10/tests/compiler/dart2j...
File tests/compiler/dart2js/unparser_test.dart (right):

https://chromiumcodereview.appspot.com/10891004/diff/10/tests/compiler/dart2j...
tests/compiler/dart2js/unparser_test.dart:785: testDeclarationTypePlaceholders()
{
On 2012/08/28 13:06:32, Anton Muhin wrote:
> why some complicated?  why not pass a flag to omit type and check against
> expected output?

Done.

[Anton Muhin, 2012-08-28 13:37:23]

https://chromiumcodereview.appspot.com/10891004/diff/1/lib/compiler/implement...
File lib/compiler/implementation/dart_backend/placeholder_collector.dart
(right):

https://chromiumcodereview.appspot.com/10891004/diff/1/lib/compiler/implement...
lib/compiler/implementation/dart_backend/placeholder_collector.dart:229: void
makeDeclarationTypePlaceholder(TypeAnnotation type, bool canOmitType) {
I like the idea with renaming


On 2012/08/28 13:27:51, Roman wrote:
> On 2012/08/28 13:06:32, Anton Muhin wrote:
> > That's not the matter of saving argument, but the fact that in all other
> places
> > types can be omitted which is now immediately seen.
> > 
> > On 2012/08/28 12:49:30, Roman wrote:
> > > On 2012/08/28 09:13:37, Anton Muhin wrote:
> > > > it looks like the only place where canOmitType is true is
> > > > makeVarDeclarationPlaceholder.  Given that most probably node.type in
this
> > > case
> > > > cannot be null, can you drop this argument and inline necessary logici
nto
> > > > makeVDTP?
> > > 
> > > I can, but I don't see any benefits. I will save one argument, but will
make
> > > code duplication in makeVarDeclarationTypePlaceholder, also method
> > > makeDeclarationTypePlaceholder would hide that the meaning is that I can
> omit
> > > type, I will need to rename it.
> > 
> 
> What about renaming this method to
> makeOmitDeclarationTypePlaceholder(TypeAnnotation type) ? I want either this
> method name to clearly tell that type can be omitted or have a parameter for
> omitting.

https://chromiumcodereview.appspot.com/10891004/diff/1/lib/compiler/implement...
lib/compiler/implementation/dart_backend/placeholder_collector.dart:421: for
(Link<Node> link = parameters.nodes; !link.isEmpty(); link= link.tail) {
Sorry, with for loop and inner dispatch, one in your comments.

On 2012/08/28 13:27:51, Roman wrote:
> On 2012/08/28 13:06:32, Anton Muhin wrote:
> > Yes, this looks more readable to me.
> > 
> > On 2012/08/28 12:49:30, Roman wrote:
> > > On 2012/08/28 09:13:37, Anton Muhin wrote:
> > > > for (Node parameter in parameters) ?
> > > 
> > > Can't do this, the structure is: first several Node-s are
> VariableDefinitions
> > > representing required parameters, then there's a NodeList that has
> > > VariableDefinitions representing optional parameters.
> > > Another option is to do something like this:
> > > 
> > > for(Node node in parameters.nodes) {
> > >   if (node is NodeList) {
> > >     // Optional parameters.
> > >     collectFunctionParameters(node);
> > >   } else {
> > >     // Required parameter.
> > >     makeDeclarationTypePlaceholder...
> > >   }
> > > }
> > 
> 
> Sorry, which one looks more readable?

[Anton Muhin, 2012-08-28 13:39:45]

https://chromiumcodereview.appspot.com/10891004/diff/11/tests/compiler/dart2j...
File tests/compiler/dart2js/unparser_test.dart (right):

https://chromiumcodereview.appspot.com/10891004/diff/11/tests/compiler/dart2j...
tests/compiler/dart2js/unparser_test.dart:12:
#import("../../../lib/compiler/implementation/scanner/scannerlib.dart");
why?

[Roman, 2012-08-28 14:48:55]

https://chromiumcodereview.appspot.com/10891004/diff/1/lib/compiler/implement...
File lib/compiler/implementation/dart_backend/placeholder_collector.dart
(right):

https://chromiumcodereview.appspot.com/10891004/diff/1/lib/compiler/implement...
lib/compiler/implementation/dart_backend/placeholder_collector.dart:229: void
makeDeclarationTypePlaceholder(TypeAnnotation type, bool canOmitType) {
On 2012/08/28 13:37:23, Anton Muhin wrote:
> I like the idea with renaming
> 
> 
> On 2012/08/28 13:27:51, Roman wrote:
> > On 2012/08/28 13:06:32, Anton Muhin wrote:
> > > That's not the matter of saving argument, but the fact that in all other
> > places
> > > types can be omitted which is now immediately seen.
> > > 
> > > On 2012/08/28 12:49:30, Roman wrote:
> > > > On 2012/08/28 09:13:37, Anton Muhin wrote:
> > > > > it looks like the only place where canOmitType is true is
> > > > > makeVarDeclarationPlaceholder.  Given that most probably node.type in
> this
> > > > case
> > > > > cannot be null, can you drop this argument and inline necessary logici
> nto
> > > > > makeVDTP?
> > > > 
> > > > I can, but I don't see any benefits. I will save one argument, but will
> make
> > > > code duplication in makeVarDeclarationTypePlaceholder, also method
> > > > makeDeclarationTypePlaceholder would hide that the meaning is that I can
> > omit
> > > > type, I will need to rename it.
> > > 
> > 
> > What about renaming this method to
> > makeOmitDeclarationTypePlaceholder(TypeAnnotation type) ? I want either this
> > method name to clearly tell that type can be omitted or have a parameter for
> > omitting.
> 

Done.

https://chromiumcodereview.appspot.com/10891004/diff/1/lib/compiler/implement...
lib/compiler/implementation/dart_backend/placeholder_collector.dart:421: for
(Link<Node> link = parameters.nodes; !link.isEmpty(); link= link.tail) {
On 2012/08/28 13:37:23, Anton Muhin wrote:
> Sorry, with for loop and inner dispatch, one in your comments.
> 
> On 2012/08/28 13:27:51, Roman wrote:
> > On 2012/08/28 13:06:32, Anton Muhin wrote:
> > > Yes, this looks more readable to me.
> > > 
> > > On 2012/08/28 12:49:30, Roman wrote:
> > > > On 2012/08/28 09:13:37, Anton Muhin wrote:
> > > > > for (Node parameter in parameters) ?
> > > > 
> > > > Can't do this, the structure is: first several Node-s are
> > VariableDefinitions
> > > > representing required parameters, then there's a NodeList that has
> > > > VariableDefinitions representing optional parameters.
> > > > Another option is to do something like this:
> > > > 
> > > > for(Node node in parameters.nodes) {
> > > >   if (node is NodeList) {
> > > >     // Optional parameters.
> > > >     collectFunctionParameters(node);
> > > >   } else {
> > > >     // Required parameter.
> > > >     makeDeclarationTypePlaceholder...
> > > >   }
> > > > }
> > > 
> > 
> > Sorry, which one looks more readable?
> 

Done.

https://chromiumcodereview.appspot.com/10891004/diff/11/tests/compiler/dart2j...
File tests/compiler/dart2js/unparser_test.dart (right):

https://chromiumcodereview.appspot.com/10891004/diff/11/tests/compiler/dart2j...
tests/compiler/dart2js/unparser_test.dart:12:
#import("../../../lib/compiler/implementation/scanner/scannerlib.dart");
On 2012/08/28 13:39:45, Anton Muhin wrote:
> why?

Thanks, now obsolete.

[Anton Muhin, 2012-08-28 14:49:58]

LGTM!