support basic identifiers in constructors

support basic identifiers in constructors

R=sigmund@google.com

Committed: https://github.com/dart-lang/static_init/commit/b26d9c704c29892a470bddc17a9b853ec3115cb4

[jakemac, 2015-01-22 19:17:33]

jakemac@google.com changed reviewers:
+ sigmund@google.com

[jakemac, 2015-01-22 19:17:34]

resolves https://github.com/dart-lang/initialize/issues/5

[Siggi Cherem (dart-lang), 2015-01-23 17:56:28]

nice!

https://codereview.chromium.org/865293002/diff/40001/lib/transformer.dart
File lib/transformer.dart (right):

https://codereview.chromium.org/865293002/diff/40001/lib/transformer.dart#new...
lib/transformer.dart:327: buffer.write((expression as
dynamic).value.toString());
nit: use the shorter '${e}' instead of e.toString() :-)

https://codereview.chromium.org/865293002/diff/40001/lib/transformer.dart#new...
lib/transformer.dart:328: } else if (expression is NullLiteral) {
you can include this with boolean, double and int, in dart null.toString is
'null'

https://codereview.chromium.org/865293002/diff/40001/lib/transformer.dart#new...
lib/transformer.dart:332: if (constKeyword != null) buffer.write('$constKeyword
');
it should always be const. If it's not const, the analyzer will give an error
somewhere, we could also complain here if we want. I think it should be fine to
replace these two lines with just:

  buffer.write('const ');

https://codereview.chromium.org/865293002/diff/40001/lib/transformer.dart#new...
lib/transformer.dart:343: if (constKeyword != null) buffer.write('$constKeyword
');
same here

https://codereview.chromium.org/865293002/diff/40001/test/common.dart
File test/common.dart (right):

https://codereview.chromium.org/865293002/diff/40001/test/common.dart#newcode30
test/common.dart:30: class DynamicInit extends Initializer<dynamic> {
I think we should keep this mockInitialize as a subset of initialize.dart, so
I'd move DynamicInit somewhere else in the tests.

https://codereview.chromium.org/865293002/diff/40001/test/transformer_test.dart
File test/transformer_test.dart (right):

https://codereview.chromium.org/865293002/diff/40001/test/transformer_test.da...
test/transformer_test.dart:82: @DynamicInit([foo, Foo.foo, 'foo'])
this would become invalid :)

https://codereview.chromium.org/865293002/diff/40001/test/transformer_test.da...
test/transformer_test.dart:83: @DynamicInit(const [foo, Foo.foo, 'foo'])
DynamicInit takes a String argument, maybe change the type to dynamic?

Also, let's add a test for named arguments:

@NamedArgInit(arg1, name: arg2)

[Siggi Cherem (dart-lang), 2015-01-23 17:57:57]

btw - it would be nice to coverge with the code in smoke at some point, maybe we
should move the code in smoke to a shared package for codegen... let's add a bug
somewhere so we don't forget about this

[jakemac, 2015-01-23 21:33:25]

On 2015/01/23 17:57:57, Siggi Cherem (dart-lang) wrote:
> btw - it would be nice to coverge with the code in smoke at some point, maybe
we
> should move the code in smoke to a shared package for codegen... let's add a
bug
> somewhere so we don't forget about this

Ya, I agree that it would be nice as they are very similar. The way they work is
pretty different but we should be able to work something out.

[jakemac, 2015-01-23 21:34:39]

https://codereview.chromium.org/865293002/diff/40001/lib/transformer.dart
File lib/transformer.dart (right):

https://codereview.chromium.org/865293002/diff/40001/lib/transformer.dart#new...
lib/transformer.dart:327: buffer.write((expression as
dynamic).value.toString());
On 2015/01/23 17:56:28, Siggi Cherem (dart-lang) wrote:
> nit: use the shorter '${e}' instead of e.toString() :-)

Done.

https://codereview.chromium.org/865293002/diff/40001/lib/transformer.dart#new...
lib/transformer.dart:328: } else if (expression is NullLiteral) {
On 2015/01/23 17:56:28, Siggi Cherem (dart-lang) wrote:
> you can include this with boolean, double and int, in dart null.toString is
> 'null'

Done, I originally had it because of the .value access above (NullLiteral
doesn't have a value) but it looks like I didn't need to access .value at all,
the regular toString of these literals just does the right thing.

https://codereview.chromium.org/865293002/diff/40001/lib/transformer.dart#new...
lib/transformer.dart:332: if (constKeyword != null) buffer.write('$constKeyword
');
On 2015/01/23 17:56:28, Siggi Cherem (dart-lang) wrote:
> it should always be const. If it's not const, the analyzer will give an error
> somewhere, we could also complain here if we want. I think it should be fine
to
> replace these two lines with just:
> 
>   buffer.write('const ');

Done.

https://codereview.chromium.org/865293002/diff/40001/lib/transformer.dart#new...
lib/transformer.dart:343: if (constKeyword != null) buffer.write('$constKeyword
');
On 2015/01/23 17:56:28, Siggi Cherem (dart-lang) wrote:
> same here

Done.

https://codereview.chromium.org/865293002/diff/40001/test/common.dart
File test/common.dart (right):

https://codereview.chromium.org/865293002/diff/40001/test/common.dart#newcode30
test/common.dart:30: class DynamicInit extends Initializer<dynamic> {
On 2015/01/23 17:56:28, Siggi Cherem (dart-lang) wrote:
> I think we should keep this mockInitialize as a subset of initialize.dart, so
> I'd move DynamicInit somewhere else in the tests.

Done.

https://codereview.chromium.org/865293002/diff/40001/test/transformer_test.dart
File test/transformer_test.dart (right):

https://codereview.chromium.org/865293002/diff/40001/test/transformer_test.da...
test/transformer_test.dart:82: @DynamicInit([foo, Foo.foo, 'foo'])
On 2015/01/23 17:56:28, Siggi Cherem (dart-lang) wrote:
> this would become invalid :)

Done.

https://codereview.chromium.org/865293002/diff/40001/test/transformer_test.da...
test/transformer_test.dart:83: @DynamicInit(const [foo, Foo.foo, 'foo'])
On 2015/01/23 17:56:28, Siggi Cherem (dart-lang) wrote:
> DynamicInit takes a String argument, maybe change the type to dynamic?
> 
> Also, let's add a test for named arguments:
> 
> @NamedArgInit(arg1, name: arg2)

Done.

[Siggi Cherem (dart-lang), 2015-01-23 21:44:23]

lgtm

https://codereview.chromium.org/865293002/diff/40001/lib/transformer.dart
File lib/transformer.dart (right):

https://codereview.chromium.org/865293002/diff/40001/lib/transformer.dart#new...
lib/transformer.dart:328: } else if (expression is NullLiteral) {
On 2015/01/23 21:34:38, jakemac wrote:
> On 2015/01/23 17:56:28, Siggi Cherem (dart-lang) wrote:
> > you can include this with boolean, double and int, in dart null.toString is
> > 'null'
> 
> Done, I originally had it because of the .value access above (NullLiteral
> doesn't have a value) but it looks like I didn't need to access .value at all,
> the regular toString of these literals just does the right thing.

Ah, good point, I had not seen the .value. Glad it worked though :)

[jakemac, 2015-01-23 21:57:21]

Committed patchset #7 (id:120001) manually as
b26d9c704c29892a470bddc17a9b853ec3115cb4 (presubmit successful).