Implement constant maps.

Implement constant maps.

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

[floitsch, 2012-03-09 14:11:08]

I still have to write some tests and update the status files.
(Apparently there are 2 new tests we fail now and 3 new tests that we pass).

[ngeoffray, 2012-03-09 18:08:50]

LGTM!

https://chromiumcodereview.appspot.com/9665001/diff/2001/frog/leg/compile_tim...
File frog/leg/compile_time_constants.dart (right):

https://chromiumcodereview.appspot.com/9665001/diff/2001/frog/leg/compile_tim...
frog/leg/compile_time_constants.dart:335: // class element.
Please also say that it's because we emit the order for the parameters of the JS
constructor is the order in the Dart class.

https://chromiumcodereview.appspot.com/9665001/diff/2001/frog/leg/compile_tim...
frog/leg/compile_time_constants.dart:337: ClassElement classElement =
type.element;
Share that with line 332.

https://chromiumcodereview.appspot.com/9665001/diff/2001/frog/leg/compile_tim...
frog/leg/compile_time_constants.dart:346: continue;
Please add a comment that this is for methods.

https://chromiumcodereview.appspot.com/9665001/diff/2001/frog/leg/compile_tim...
frog/leg/compile_time_constants.dart:356: handler.compiler.internalError("Could
not generate constant map");
Maybe say something more explicit like: "compiler and ConstantMap class disagree
on number of fields".

[floitsch, 2012-03-10 17:40:24]

PTAL.

https://chromiumcodereview.appspot.com/9665001/diff/2001/frog/leg/compile_tim...
File frog/leg/compile_time_constants.dart (right):

https://chromiumcodereview.appspot.com/9665001/diff/2001/frog/leg/compile_tim...
frog/leg/compile_time_constants.dart:335: // class element.
On 2012/03/09 18:08:50, ngeoffray wrote:
> Please also say that it's because we emit the order for the parameters of the
JS
> constructor is the order in the Dart class.

Done.

https://chromiumcodereview.appspot.com/9665001/diff/2001/frog/leg/compile_tim...
frog/leg/compile_time_constants.dart:337: ClassElement classElement =
type.element;
On 2012/03/09 18:08:50, ngeoffray wrote:
> Share that with line 332.

Done.

https://chromiumcodereview.appspot.com/9665001/diff/2001/frog/leg/compile_tim...
frog/leg/compile_time_constants.dart:346: continue;
On 2012/03/09 18:08:50, ngeoffray wrote:
> Please add a comment that this is for methods.

Done.

https://chromiumcodereview.appspot.com/9665001/diff/2001/frog/leg/compile_tim...
frog/leg/compile_time_constants.dart:356: handler.compiler.internalError("Could
not generate constant map");
On 2012/03/09 18:08:50, ngeoffray wrote:
> Maybe say something more explicit like: "compiler and ConstantMap class
disagree
> on number of fields".

Done.

[ngeoffray, 2012-03-10 18:33:55]

LGTM, but please consider adding an abstraction for the dependencies.

https://chromiumcodereview.appspot.com/9665001/diff/5011/frog/leg/compile_tim...
File frog/leg/compile_time_constants.dart (right):

https://chromiumcodereview.appspot.com/9665001/diff/5011/frog/leg/compile_tim...
frog/leg/compile_time_constants.dart:26: * already in the [seenConstants] list.
Does not add itself to the [list].
[seenConstants] list -> [seenConstants] set.

https://chromiumcodereview.appspot.com/9665001/diff/5011/frog/leg/compile_tim...
frog/leg/compile_time_constants.dart:34: abstract void
addDependencies(List<Constant> list,
Maybe rename list to orderedList. 'list' is too generic here and a better name
would explain better what this method is doing.

https://chromiumcodereview.appspot.com/9665001/diff/5011/frog/leg/compile_tim...
frog/leg/compile_time_constants.dart:258: void addDependency(Constant constant,
Instead of putting addDependeny on a Constant class, I suggest creating an
abstraction for the dependencies. It would encapsulate the list and and the set
and contain this method.

Also, the addDependencies could be changed to a getDependecies method that
returns a list. The suggested abstraction would then use this list to update
itself.

https://chromiumcodereview.appspot.com/9665001/diff/5011/frog/leg/compile_tim...
frog/leg/compile_time_constants.dart:770: entry.key.asStringInterpolation() ===
null)) {
AFAIK, string interpolation for constants is still not in the spec.

[floitsch, 2012-03-10 19:58:54]

PTA (hopefully final) L. :)

https://chromiumcodereview.appspot.com/9665001/diff/5011/frog/leg/compile_tim...
File frog/leg/compile_time_constants.dart (right):

https://chromiumcodereview.appspot.com/9665001/diff/5011/frog/leg/compile_tim...
frog/leg/compile_time_constants.dart:26: * already in the [seenConstants] list.
Does not add itself to the [list].
On 2012/03/10 18:33:55, ngeoffray wrote:
> [seenConstants] list -> [seenConstants] set.

has been removed.

https://chromiumcodereview.appspot.com/9665001/diff/5011/frog/leg/compile_tim...
frog/leg/compile_time_constants.dart:34: abstract void
addDependencies(List<Constant> list,
On 2012/03/10 18:33:55, ngeoffray wrote:
> Maybe rename list to orderedList. 'list' is too generic here and a better name
> would explain better what this method is doing.

has been removed.

https://chromiumcodereview.appspot.com/9665001/diff/5011/frog/leg/compile_tim...
frog/leg/compile_time_constants.dart:258: void addDependency(Constant constant,
On 2012/03/10 18:33:55, ngeoffray wrote:
> Instead of putting addDependeny on a Constant class, I suggest creating an
> abstraction for the dependencies. It would encapsulate the list and and the
set
> and contain this method.
> 
> Also, the addDependencies could be changed to a getDependecies method that
> returns a list. The suggested abstraction would then use this list to update
> itself.

replaced with getDependencies().

https://chromiumcodereview.appspot.com/9665001/diff/5011/frog/leg/compile_tim...
frog/leg/compile_time_constants.dart:770: entry.key.asStringInterpolation() ===
null)) {
On 2012/03/10 18:33:55, ngeoffray wrote:
> AFAIK, string interpolation for constants is still not in the spec.

This is in preparation for future changes.
Since string-interpolation is not a compile-time constant the key will return as
'null'.
In some sense the check is currently dead code. If you want I can remove it for
now.

[ngeoffray, 2012-03-10 22:28:03]

STV!

https://chromiumcodereview.appspot.com/9665001/diff/5011/frog/leg/compile_tim...
File frog/leg/compile_time_constants.dart (right):

https://chromiumcodereview.appspot.com/9665001/diff/5011/frog/leg/compile_tim...
frog/leg/compile_time_constants.dart:770: entry.key.asStringInterpolation() ===
null)) {
On 2012/03/10 19:58:54, floitsch wrote:
> On 2012/03/10 18:33:55, ngeoffray wrote:
> > AFAIK, string interpolation for constants is still not in the spec.
> 
> This is in preparation for future changes.
> Since string-interpolation is not a compile-time constant the key will return
as
> 'null'.
> In some sense the check is currently dead code. If you want I can remove it
for
> now.

I'd prefer that you remove it. I have a strong aversion against dead code.

https://chromiumcodereview.appspot.com/9665001/diff/5014/tests/co19/co19-leg....
File tests/co19/co19-leg.status (right):

https://chromiumcodereview.appspot.com/9665001/diff/5014/tests/co19/co19-leg....
tests/co19/co19-leg.status:210: Language/10_Expressions/01_Constants_A15_t03:
Fail # 'const' keyword is not verified in literal maps.
This can be a Fail, OK for Q1 because dartc will check it (there is a list of
Fail, OK in the file).

https://chromiumcodereview.appspot.com/9665001/diff/5014/tests/language/langu...
File tests/language/language-leg.status (right):

https://chromiumcodereview.appspot.com/9665001/diff/5014/tests/language/langu...
tests/language/language-leg.status:31: ConstOptionalArgsNegativeTest: Fail #
'const' keyword is not verified in literal maps.
ditto

[floitsch, 2012-03-10 22:58:22]

https://chromiumcodereview.appspot.com/9665001/diff/5011/frog/leg/compile_tim...
File frog/leg/compile_time_constants.dart (right):

https://chromiumcodereview.appspot.com/9665001/diff/5011/frog/leg/compile_tim...
frog/leg/compile_time_constants.dart:770: entry.key.asStringInterpolation() ===
null)) {
On 2012/03/10 22:28:03, ngeoffray wrote:
> On 2012/03/10 19:58:54, floitsch wrote:
> > On 2012/03/10 18:33:55, ngeoffray wrote:
> > > AFAIK, string interpolation for constants is still not in the spec.
> > 
> > This is in preparation for future changes.
> > Since string-interpolation is not a compile-time constant the key will
return
> as
> > 'null'.
> > In some sense the check is currently dead code. If you want I can remove it
> for
> > now.
> 
> I'd prefer that you remove it. I have a strong aversion against dead code.

Done.

https://chromiumcodereview.appspot.com/9665001/diff/5014/tests/co19/co19-leg....
File tests/co19/co19-leg.status (right):

https://chromiumcodereview.appspot.com/9665001/diff/5014/tests/co19/co19-leg....
tests/co19/co19-leg.status:210: Language/10_Expressions/01_Constants_A15_t03:
Fail # 'const' keyword is not verified in literal maps.
On 2012/03/10 22:28:03, ngeoffray wrote:
> This can be a Fail, OK for Q1 because dartc will check it (there is a list of
> Fail, OK in the file).

Done.

https://chromiumcodereview.appspot.com/9665001/diff/5014/tests/language/langu...
File tests/language/language-leg.status (right):

https://chromiumcodereview.appspot.com/9665001/diff/5014/tests/language/langu...
tests/language/language-leg.status:31: ConstOptionalArgsNegativeTest: Fail #
'const' keyword is not verified in literal maps.
On 2012/03/10 22:28:03, ngeoffray wrote:
> ditto

Done.