Lazy implementation of final variables.

Lazy implementation of final variables.

Fixes issue 3559.

Depends on CL https://chromiumcodereview.appspot.com/10832351/

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

[floitsch, 2012-08-15 14:22:35]

Still missing tests.
And there are some tests that are now failing or passing. I need to go through
them one by one. I hope that most of them just need a status-file update,
though.

[floitsch, 2012-08-15 20:55:51]

All tests are passing now.
Still missing new tests for the added functionality.

[kasperl, 2012-08-16 14:41:37]

First round of comments. I still need to spend more time looking at the tests:

https://chromiumcodereview.appspot.com/10855174/diff/7001/lib/compiler/implem...
File lib/compiler/implementation/compile_time_constants.dart (right):

https://chromiumcodereview.appspot.com/10855174/diff/7001/lib/compiler/implem...
lib/compiler/implementation/compile_time_constants.dart:491: /** Map from
compile-time constants to their JS name.*/
Space between . and */

https://chromiumcodereview.appspot.com/10855174/diff/7001/lib/compiler/implem...
lib/compiler/implementation/compile_time_constants.dart:494: /** The set of
variable elements that are in the process of being computed.*/
Space between . and */

https://chromiumcodereview.appspot.com/10855174/diff/7001/lib/compiler/implem...
lib/compiler/implementation/compile_time_constants.dart:517: * statically.
Returns `null` if the variable must be initialized lazily.
`null` -- back ticks, really?

https://chromiumcodereview.appspot.com/10855174/diff/7001/lib/compiler/implem...
File lib/compiler/implementation/compiler.dart (right):

https://chromiumcodereview.appspot.com/10855174/diff/7001/lib/compiler/implem...
lib/compiler/implementation/compiler.dart:754: // Otherwise we need to go
through the builder.
Maybe change the comment to explain why we have to go through the builder for
this.

https://chromiumcodereview.appspot.com/10855174/diff/7001/lib/compiler/implem...
lib/compiler/implementation/compiler.dart:798: return initialValue == null;
===?

https://chromiumcodereview.appspot.com/10855174/diff/7001/lib/compiler/implem...
File lib/compiler/implementation/elements/elements.dart (right):

https://chromiumcodereview.appspot.com/10855174/diff/7001/lib/compiler/implem...
lib/compiler/implementation/elements/elements.dart:163: && (modifiers.isFinal()
|| modifiers.isConst())) return false;
Maybe introduce a modifiers.isFinalOrConst()?

https://chromiumcodereview.appspot.com/10855174/diff/7001/lib/compiler/implem...
File lib/compiler/implementation/js_backend/emitter.dart (right):

https://chromiumcodereview.appspot.com/10855174/diff/7001/lib/compiler/implem...
lib/compiler/implementation/js_backend/emitter.dart:659: if
(!element.isInstanceMember() && element.kind != ElementKind.FIELD) {
Don't we have an element.isField() tester?

https://chromiumcodereview.appspot.com/10855174/diff/7001/lib/compiler/implem...
lib/compiler/implementation/js_backend/emitter.dart:839: if (element.kind ==
ElementKind.FIELD) {
isField

https://chromiumcodereview.appspot.com/10855174/diff/7001/lib/compiler/implem...
lib/compiler/implementation/js_backend/emitter.dart:847: if (element.kind ==
ElementKind.FIELD) {
isField

https://chromiumcodereview.appspot.com/10855174/diff/7001/lib/compiler/implem...
lib/compiler/implementation/js_backend/emitter.dart:877: $lazyGetterName =
function() {
As we discussed, I think this needs to do the try-catch dance. Maybe you can add
a helper function (parameterized with a closure or the field name) that deals
with that to make the code shorter?

https://chromiumcodereview.appspot.com/10855174/diff/7001/lib/compiler/implem...
File lib/compiler/implementation/ssa/builder.dart (right):

https://chromiumcodereview.appspot.com/10855174/diff/7001/lib/compiler/implem...
lib/compiler/implementation/ssa/builder.dart:1792: if (element.kind ==
ElementKind.FIELD && !element.isAssignable()) {
isField?

https://chromiumcodereview.appspot.com/10855174/diff/7001/lib/compiler/implem...
lib/compiler/implementation/ssa/builder.dart:1801: assert(element.kind ==
ElementKind.FIELD);
Seems like a completely trivial assert given the condition.

https://chromiumcodereview.appspot.com/10855174/diff/7001/tests/compiler/dart...
File tests/compiler/dart2js_extra/compile_time_constant4_test.dart (left):

https://chromiumcodereview.appspot.com/10855174/diff/7001/tests/compiler/dart...
tests/compiler/dart2js_extra/compile_time_constant4_test.dart:1: // Copyright
(c) 2012, the Dart project authors.  Please see the AUTHORS file
Couldn't you keep this and change the finals to const?

[floitsch, 2012-08-16 22:52:33]

https://chromiumcodereview.appspot.com/10855174/diff/7001/lib/compiler/implem...
File lib/compiler/implementation/compile_time_constants.dart (right):

https://chromiumcodereview.appspot.com/10855174/diff/7001/lib/compiler/implem...
lib/compiler/implementation/compile_time_constants.dart:491: /** Map from
compile-time constants to their JS name.*/
On 2012/08/16 14:41:37, kasperl wrote:
> Space between . and */

Done.

https://chromiumcodereview.appspot.com/10855174/diff/7001/lib/compiler/implem...
lib/compiler/implementation/compile_time_constants.dart:494: /** The set of
variable elements that are in the process of being computed.*/
On 2012/08/16 14:41:37, kasperl wrote:
> Space between . and */

Done.

https://chromiumcodereview.appspot.com/10855174/diff/7001/lib/compiler/implem...
lib/compiler/implementation/compile_time_constants.dart:517: * statically.
Returns `null` if the variable must be initialized lazily.
On 2012/08/16 14:41:37, kasperl wrote:
> `null` -- back ticks, really?
I find it nice to read and it's the code-tag in mark-down.
Changed to [:null:].

https://chromiumcodereview.appspot.com/10855174/diff/7001/lib/compiler/implem...
File lib/compiler/implementation/compiler.dart (right):

https://chromiumcodereview.appspot.com/10855174/diff/7001/lib/compiler/implem...
lib/compiler/implementation/compiler.dart:754: // Otherwise we need to go
through the builder.
On 2012/08/16 14:41:37, kasperl wrote:
> Maybe change the comment to explain why we have to go through the builder for
> this.

Done.

https://chromiumcodereview.appspot.com/10855174/diff/7001/lib/compiler/implem...
lib/compiler/implementation/compiler.dart:798: return initialValue == null;
On 2012/08/16 14:41:37, kasperl wrote:
> ===?

Done.

https://chromiumcodereview.appspot.com/10855174/diff/7001/lib/compiler/implem...
File lib/compiler/implementation/elements/elements.dart (right):

https://chromiumcodereview.appspot.com/10855174/diff/7001/lib/compiler/implem...
lib/compiler/implementation/elements/elements.dart:163: && (modifiers.isFinal()
|| modifiers.isConst())) return false;
On 2012/08/16 14:41:37, kasperl wrote:
> Maybe introduce a modifiers.isFinalOrConst()? 

Done.

https://chromiumcodereview.appspot.com/10855174/diff/7001/lib/compiler/implem...
File lib/compiler/implementation/js_backend/emitter.dart (right):

https://chromiumcodereview.appspot.com/10855174/diff/7001/lib/compiler/implem...
lib/compiler/implementation/js_backend/emitter.dart:659: if
(!element.isInstanceMember() && element.kind != ElementKind.FIELD) {
On 2012/08/16 14:41:37, kasperl wrote:
> Don't we have an element.isField() tester?

Done.

https://chromiumcodereview.appspot.com/10855174/diff/7001/lib/compiler/implem...
lib/compiler/implementation/js_backend/emitter.dart:839: if (element.kind ==
ElementKind.FIELD) {
On 2012/08/16 14:41:37, kasperl wrote:
> isField

Done.

https://chromiumcodereview.appspot.com/10855174/diff/7001/lib/compiler/implem...
lib/compiler/implementation/js_backend/emitter.dart:847: if (element.kind ==
ElementKind.FIELD) {
On 2012/08/16 14:41:37, kasperl wrote:
> isField

Done.

https://chromiumcodereview.appspot.com/10855174/diff/7001/lib/compiler/implem...
lib/compiler/implementation/js_backend/emitter.dart:877: $lazyGetterName =
function() {
On 2012/08/16 14:41:37, kasperl wrote:
> As we discussed, I think this needs to do the try-catch dance. Maybe you can
add
> a helper function (parameterized with a closure or the field name) that deals
> with that to make the code shorter?

Done.

https://chromiumcodereview.appspot.com/10855174/diff/7001/lib/compiler/implem...
File lib/compiler/implementation/ssa/builder.dart (right):

https://chromiumcodereview.appspot.com/10855174/diff/7001/lib/compiler/implem...
lib/compiler/implementation/ssa/builder.dart:1792: if (element.kind ==
ElementKind.FIELD && !element.isAssignable()) {
On 2012/08/16 14:41:37, kasperl wrote:
> isField?

Done.

https://chromiumcodereview.appspot.com/10855174/diff/7001/lib/compiler/implem...
lib/compiler/implementation/ssa/builder.dart:1801: assert(element.kind ==
ElementKind.FIELD);
On 2012/08/16 14:41:37, kasperl wrote:
> Seems like a completely trivial assert given the condition.

Done.

https://chromiumcodereview.appspot.com/10855174/diff/7001/tests/compiler/dart...
File tests/compiler/dart2js_extra/compile_time_constant4_test.dart (left):

https://chromiumcodereview.appspot.com/10855174/diff/7001/tests/compiler/dart...
tests/compiler/dart2js_extra/compile_time_constant4_test.dart:1: // Copyright
(c) 2012, the Dart project authors.  Please see the AUTHORS file
On 2012/08/16 14:41:37, kasperl wrote:
> Couldn't you keep this and change the finals to const?

This test was broken: it used string-plus. I didn't see the value of the test.
The coverage in language is pretty good.

[kasperl, 2012-08-17 09:30:04]

LGTM, but it would be nice with test for the exceptional cases where the lazy
initialization expression throws an exception -- and possibly also for the cases
where we get cyclic initialization.

https://chromiumcodereview.appspot.com/10855174/diff/12001/lib/compiler/imple...
File lib/compiler/implementation/compile_time_constants.dart (right):

https://chromiumcodereview.appspot.com/10855174/diff/12001/lib/compiler/imple...
lib/compiler/implementation/compile_time_constants.dart:528: Constant result;
Constant result = initial... ?

https://chromiumcodereview.appspot.com/10855174/diff/12001/lib/compiler/imple...
lib/compiler/implementation/compile_time_constants.dart:1114: void
signalNotACompileTimeConstant(Node node) {
I would remove the A.

https://chromiumcodereview.appspot.com/10855174/diff/12001/lib/compiler/imple...
lib/compiler/implementation/compile_time_constants.dart:1120: // sometimes see
non-compile time constants.
So in this case, you'll end up returning null anyway, right? You could make that
link more obvious by returning the result of calling
signalNotCompileTimeConstant where you use it and make this return a Constant
(which will always be null I guess).

https://chromiumcodereview.appspot.com/10855174/diff/12001/lib/compiler/imple...
File lib/compiler/implementation/compiler.dart (right):

https://chromiumcodereview.appspot.com/10855174/diff/12001/lib/compiler/imple...
lib/compiler/implementation/compiler.dart:769: if (initialValue != null) return;
!==

https://chromiumcodereview.appspot.com/10855174/diff/12001/lib/compiler/imple...
File lib/compiler/implementation/js_backend/emitter.dart (right):

https://chromiumcodereview.appspot.com/10855174/diff/12001/lib/compiler/imple...
lib/compiler/implementation/js_backend/emitter.dart:265: if (result ===
sentinel) {
I guess this doesn't catch cyclic initialization. I just keeps on truckin'.

https://chromiumcodereview.appspot.com/10855174/diff/12001/lib/compiler/imple...
File lib/compiler/implementation/ssa/builder.dart (right):

https://chromiumcodereview.appspot.com/10855174/diff/12001/lib/compiler/imple...
lib/compiler/implementation/ssa/builder.dart:3253: // condition
Terminate comment with .

https://chromiumcodereview.appspot.com/10855174/diff/12001/lib/compiler/imple...
File lib/compiler/implementation/ssa/nodes.dart (right):

https://chromiumcodereview.appspot.com/10855174/diff/12001/lib/compiler/imple...
lib/compiler/implementation/ssa/nodes.dart:2264: setAllSideEffects();
Can't you just enable GVN for the lazy-static and keep it having all side
effects? It would be neat if we could read it into a local variable on first
access (and kill stuff in our GVN map) but then just use the local after that.

https://chromiumcodereview.appspot.com/10855174/diff/12001/tests/language/lan...
File tests/language/language_dart2js.status (right):

https://chromiumcodereview.appspot.com/10855174/diff/12001/tests/language/lan...
tests/language/language_dart2js.status:86: lazy_final2_test: Fail # Issue 3559 
No support for closures in lazy initializers.
Add : after issue no.?

https://chromiumcodereview.appspot.com/10855174/diff/12001/tests/language/lan...
tests/language/language_dart2js.status:242: final_syntax_test/03: Fail, OK
Maybe comment on why we're okay with failing these.

https://chromiumcodereview.appspot.com/10855174/diff/12001/tests/language/laz...
File tests/language/lazy_final_test.dart (right):

https://chromiumcodereview.appspot.com/10855174/diff/12001/tests/language/laz...
tests/language/lazy_final_test.dart:5: final x = foo();
I guess we still need tests for the exceptional cases.

[floitsch, 2012-09-04 17:32:21]

PTAL, in particular the exception handling.

Also added Peter to verify the cyclic-throw handling: 
 compiler.dart:730, 871
 resolver.dart

https://chromiumcodereview.appspot.com/10855174/diff/12001/lib/compiler/imple...
File lib/compiler/implementation/compile_time_constants.dart (right):

https://chromiumcodereview.appspot.com/10855174/diff/12001/lib/compiler/imple...
lib/compiler/implementation/compile_time_constants.dart:528: Constant result;
On 2012/08/17 09:30:04, kasperl wrote:
> Constant result = initial... ?

Done.

https://chromiumcodereview.appspot.com/10855174/diff/12001/lib/compiler/imple...
lib/compiler/implementation/compile_time_constants.dart:1114: void
signalNotACompileTimeConstant(Node node) {
On 2012/08/17 09:30:04, kasperl wrote:
> I would remove the A.

Done.

https://chromiumcodereview.appspot.com/10855174/diff/12001/lib/compiler/imple...
lib/compiler/implementation/compile_time_constants.dart:1120: // sometimes see
non-compile time constants.
On 2012/08/17 09:30:04, kasperl wrote:
> So in this case, you'll end up returning null anyway, right? You could make
that
> link more obvious by returning the result of calling
> signalNotCompileTimeConstant where you use it and make this return a Constant
> (which will always be null I guess).

Done.

https://chromiumcodereview.appspot.com/10855174/diff/12001/lib/compiler/imple...
File lib/compiler/implementation/compiler.dart (right):

https://chromiumcodereview.appspot.com/10855174/diff/12001/lib/compiler/imple...
lib/compiler/implementation/compiler.dart:769: if (initialValue != null) return;
On 2012/08/17 09:30:04, kasperl wrote:
> !==

Done.

https://chromiumcodereview.appspot.com/10855174/diff/12001/lib/compiler/imple...
File lib/compiler/implementation/js_backend/emitter.dart (right):

https://chromiumcodereview.appspot.com/10855174/diff/12001/lib/compiler/imple...
lib/compiler/implementation/js_backend/emitter.dart:265: if (result ===
sentinel) {
On 2012/08/17 09:30:04, kasperl wrote:
> I guess this doesn't catch cyclic initialization. I just keeps on truckin'.

Done.

https://chromiumcodereview.appspot.com/10855174/diff/12001/lib/compiler/imple...
File lib/compiler/implementation/ssa/builder.dart (right):

https://chromiumcodereview.appspot.com/10855174/diff/12001/lib/compiler/imple...
lib/compiler/implementation/ssa/builder.dart:3253: // condition
On 2012/08/17 09:30:04, kasperl wrote:
> Terminate comment with .

Done.

https://chromiumcodereview.appspot.com/10855174/diff/12001/lib/compiler/imple...
File lib/compiler/implementation/ssa/nodes.dart (right):

https://chromiumcodereview.appspot.com/10855174/diff/12001/lib/compiler/imple...
lib/compiler/implementation/ssa/nodes.dart:2264: setAllSideEffects();
On 2012/08/17 09:30:04, kasperl wrote:
> Can't you just enable GVN for the lazy-static and keep it having all side
> effects? It would be neat if we could read it into a local variable on first
> access (and kill stuff in our GVN map) but then just use the local after that.

I tried, but iirc there are some assumptions and asserts.

https://chromiumcodereview.appspot.com/10855174/diff/12001/tests/language/lan...
File tests/language/language_dart2js.status (right):

https://chromiumcodereview.appspot.com/10855174/diff/12001/tests/language/lan...
tests/language/language_dart2js.status:86: lazy_final2_test: Fail # Issue 3559 
No support for closures in lazy initializers.
On 2012/08/17 09:30:04, kasperl wrote:
> Add : after issue no.?

Done.

https://chromiumcodereview.appspot.com/10855174/diff/12001/tests/language/lan...
tests/language/language_dart2js.status:242: final_syntax_test/03: Fail, OK
On 2012/08/17 09:30:04, kasperl wrote:
> Maybe comment on why we're okay with failing these.

Removed OK, and added reason for failure.

https://chromiumcodereview.appspot.com/10855174/diff/12001/tests/language/laz...
File tests/language/lazy_final_test.dart (right):

https://chromiumcodereview.appspot.com/10855174/diff/12001/tests/language/laz...
tests/language/lazy_final_test.dart:5: final x = foo();
On 2012/08/17 09:30:04, kasperl wrote:
> I guess we still need tests for the exceptional cases.

Done.

[ahe, 2012-09-04 18:09:21]

Hi Florian,

Took a quick look to give you quick feedback. Not sure it's of much help :-(

I'm not sure I understand the changes to the resolver. I think we register all
helpers unconditionally, but perhaps I'm wrong and this only applies to
interceptors. 

In case you do need to register this helper, I'm not sure I understand why the
condition is sufficient or necessary. So a comment would be nice.

Cheers,
Peter

[kasperl, 2012-09-05 08:25:24]

Almost done with the review -- looks pretty good so far. I'll do a pass over the
tests and send you feedback on that shortly.

https://chromiumcodereview.appspot.com/10855174/diff/19001/lib/compiler/imple...
File lib/compiler/implementation/compile_time_constants.dart (right):

https://chromiumcodereview.appspot.com/10855174/diff/19001/lib/compiler/imple...
lib/compiler/implementation/compile_time_constants.dart:511: * initializations
of global and static const fields, and default values of
Not sure I understand this comment update. Since you now have lazy static
support, it would seem that this class also keeps track of global and static
*non-const* fields?

https://chromiumcodereview.appspot.com/10855174/diff/19001/lib/compiler/imple...
File lib/compiler/implementation/js_backend/emitter.dart (right):

https://chromiumcodereview.appspot.com/10855174/diff/19001/lib/compiler/imple...
lib/compiler/implementation/js_backend/emitter.dart:262: var sentinel_undefined
= {};
Are you deliberately using a non-JS naming convention for these locals? (using
an underscore)

https://chromiumcodereview.appspot.com/10855174/diff/19001/lib/compiler/imple...
lib/compiler/implementation/js_backend/emitter.dart:279: } else if(result ===
sentinel_in_progress) {
Add space between if and (.

https://chromiumcodereview.appspot.com/10855174/diff/19001/lib/compiler/imple...
File lib/compiler/implementation/namer.dart (right):

https://chromiumcodereview.appspot.com/10855174/diff/19001/lib/compiler/imple...
lib/compiler/implementation/namer.dart:36: // TODO(floitsch): make sure the
sentinel can't clash with other fields.
Where is this used?

https://chromiumcodereview.appspot.com/10855174/diff/19001/lib/compiler/imple...
File lib/compiler/implementation/ssa/nodes.dart (right):

https://chromiumcodereview.appspot.com/10855174/diff/19001/lib/compiler/imple...
lib/compiler/implementation/ssa/nodes.dart:2317: // TODO: The first access has
side-effects, but we afterwards we should be
File a bug for this? Milestone-Later.

https://chromiumcodereview.appspot.com/10855174/diff/19001/lib/compiler/imple...
lib/compiler/implementation/ssa/nodes.dart:2326: // TODO(floitsch): can we do
better here?
Maybe track this with the same bug.

https://chromiumcodereview.appspot.com/10855174/diff/19001/tests/language/fin...
File tests/language/final_syntax_test.dart (right):

https://chromiumcodereview.appspot.com/10855174/diff/19001/tests/language/fin...
tests/language/final_syntax_test.dart:47: // user-defined final finalructors.
Hehe. Word of the day: finalructors.

[kasperl, 2012-09-05 08:54:53]

LGTM once you've addressed Peter's concerns.

https://chromiumcodereview.appspot.com/10855174/diff/19001/tests/language/fin...
File tests/language/final_syntax_test.dart (right):

https://chromiumcodereview.appspot.com/10855174/diff/19001/tests/language/fin...
tests/language/final_syntax_test.dart:5: main() {
Should we keep an unchanged copy of this around as const_syntax_test.dart?

https://chromiumcodereview.appspot.com/10855174/diff/19001/tests/language/lan...
File tests/language/language.status (right):

https://chromiumcodereview.appspot.com/10855174/diff/19001/tests/language/lan...
tests/language/language.status:242: lazy_final_test: Fail,OK # Issue 3558
Space between , and OK (twice).

https://chromiumcodereview.appspot.com/10855174/diff/19001/tests/language/lan...
File tests/language/language_dart2js.status (right):

https://chromiumcodereview.appspot.com/10855174/diff/19001/tests/language/lan...
tests/language/language_dart2js.status:247: final_syntax_test/01: Fail # Missing
error for final unitialized field.
uninitialized final field

https://chromiumcodereview.appspot.com/10855174/diff/19001/tests/language/laz...
File tests/language/lazy_final2_test.dart (right):

https://chromiumcodereview.appspot.com/10855174/diff/19001/tests/language/laz...
tests/language/lazy_final2_test.dart:8: Expect.equals(499, x(498));
Maybe check with an extra input?

https://chromiumcodereview.appspot.com/10855174/diff/19001/tests/language/laz...
File tests/language/lazy_final3_test.dart (right):

https://chromiumcodereview.appspot.com/10855174/diff/19001/tests/language/laz...
tests/language/lazy_final3_test.dart:28: } catch(e) {
I prefer to put a space between catch and (.

https://chromiumcodereview.appspot.com/10855174/diff/19001/tests/language/laz...
tests/language/lazy_final3_test.dart:39: Expect.throws(() => fib(x));
How about adding a check that verifies that you're getting the right exception
in all the Expect.throws cases?

   Expect.throws(() => ..., (e) => e is RuntimeException);

https://chromiumcodereview.appspot.com/10855174/diff/19001/tests/language/laz...
File tests/language/lazy_final_test.dart (right):

https://chromiumcodereview.appspot.com/10855174/diff/19001/tests/language/laz...
tests/language/lazy_final_test.dart:19: side_effect() => count++;
sideEffect

[floitsch, 2012-09-05 09:31:07]

I have removed the code from the resolver.

https://chromiumcodereview.appspot.com/10855174/diff/19001/lib/compiler/imple...
File lib/compiler/implementation/compile_time_constants.dart (right):

https://chromiumcodereview.appspot.com/10855174/diff/19001/lib/compiler/imple...
lib/compiler/implementation/compile_time_constants.dart:511: * initializations
of global and static const fields, and default values of
On 2012/09/05 08:25:24, kasperl wrote:
> Not sure I understand this comment update. Since you now have lazy static
> support, it would seem that this class also keeps track of global and static
> *non-const* fields?

Removed the "const".
done.

https://chromiumcodereview.appspot.com/10855174/diff/19001/lib/compiler/imple...
File lib/compiler/implementation/js_backend/emitter.dart (right):

https://chromiumcodereview.appspot.com/10855174/diff/19001/lib/compiler/imple...
lib/compiler/implementation/js_backend/emitter.dart:262: var sentinel_undefined
= {};
On 2012/09/05 08:25:24, kasperl wrote:
> Are you deliberately using a non-JS naming convention for these locals? (using
> an underscore) 

Don't know why. Changed to JS convention.
done.

https://chromiumcodereview.appspot.com/10855174/diff/19001/lib/compiler/imple...
lib/compiler/implementation/js_backend/emitter.dart:279: } else if(result ===
sentinel_in_progress) {
On 2012/09/05 08:25:24, kasperl wrote:
> Add space between if and (.

Done.

https://chromiumcodereview.appspot.com/10855174/diff/19001/lib/compiler/imple...
File lib/compiler/implementation/namer.dart (right):

https://chromiumcodereview.appspot.com/10855174/diff/19001/lib/compiler/imple...
lib/compiler/implementation/namer.dart:36: // TODO(floitsch): make sure the
sentinel can't clash with other fields.
On 2012/09/05 08:25:24, kasperl wrote:
> Where is this used?

Removed.
done.

https://chromiumcodereview.appspot.com/10855174/diff/19001/lib/compiler/imple...
File lib/compiler/implementation/ssa/nodes.dart (right):

https://chromiumcodereview.appspot.com/10855174/diff/19001/lib/compiler/imple...
lib/compiler/implementation/ssa/nodes.dart:2317: // TODO: The first access has
side-effects, but we afterwards we should be
On 2012/09/05 08:25:24, kasperl wrote:
> File a bug for this? Milestone-Later.
done (issue 4931).

https://chromiumcodereview.appspot.com/10855174/diff/19001/lib/compiler/imple...
lib/compiler/implementation/ssa/nodes.dart:2326: // TODO(floitsch): can we do
better here?
On 2012/09/05 08:25:24, kasperl wrote:
> Maybe track this with the same bug.
ditto.

https://chromiumcodereview.appspot.com/10855174/diff/19001/tests/language/fin...
File tests/language/final_syntax_test.dart (right):

https://chromiumcodereview.appspot.com/10855174/diff/19001/tests/language/fin...
tests/language/final_syntax_test.dart:5: main() {
On 2012/09/05 08:54:54, kasperl wrote:
> Should we keep an unchanged copy of this around as const_syntax_test.dart?

This file is a copy of const_syntax_test. So there is already an unmodified
copy.

https://chromiumcodereview.appspot.com/10855174/diff/19001/tests/language/fin...
tests/language/final_syntax_test.dart:47: // user-defined final finalructors.
On 2012/09/05 08:25:24, kasperl wrote:
> Hehe. Word of the day: finalructors.
hehe.
done.

https://chromiumcodereview.appspot.com/10855174/diff/19001/tests/language/lan...
File tests/language/language.status (right):

https://chromiumcodereview.appspot.com/10855174/diff/19001/tests/language/lan...
tests/language/language.status:242: lazy_final_test: Fail,OK # Issue 3558
On 2012/09/05 08:54:54, kasperl wrote:
> Space between , and OK (twice).

Done.

https://chromiumcodereview.appspot.com/10855174/diff/19001/tests/language/lan...
File tests/language/language_dart2js.status (right):

https://chromiumcodereview.appspot.com/10855174/diff/19001/tests/language/lan...
tests/language/language_dart2js.status:247: final_syntax_test/01: Fail # Missing
error for final unitialized field.
On 2012/09/05 08:54:54, kasperl wrote:
> uninitialized final field

Done.

https://chromiumcodereview.appspot.com/10855174/diff/19001/tests/language/laz...
File tests/language/lazy_final2_test.dart (right):

https://chromiumcodereview.appspot.com/10855174/diff/19001/tests/language/laz...
tests/language/lazy_final2_test.dart:8: Expect.equals(499, x(498));
On 2012/09/05 08:54:54, kasperl wrote:
> Maybe check with an extra input?

Done.

https://chromiumcodereview.appspot.com/10855174/diff/19001/tests/language/laz...
File tests/language/lazy_final3_test.dart (right):

https://chromiumcodereview.appspot.com/10855174/diff/19001/tests/language/laz...
tests/language/lazy_final3_test.dart:28: } catch(e) {
On 2012/09/05 08:54:54, kasperl wrote:
> I prefer to put a space between catch and (.

Done.

https://chromiumcodereview.appspot.com/10855174/diff/19001/tests/language/laz...
tests/language/lazy_final3_test.dart:39: Expect.throws(() => fib(x));
On 2012/09/05 08:54:54, kasperl wrote:
> How about adding a check that verifies that you're getting the right exception
> in all the Expect.throws cases?
> 
>    Expect.throws(() => ..., (e) => e is RuntimeException);

Done.

https://chromiumcodereview.appspot.com/10855174/diff/19001/tests/language/laz...
File tests/language/lazy_final_test.dart (right):

https://chromiumcodereview.appspot.com/10855174/diff/19001/tests/language/laz...
tests/language/lazy_final_test.dart:19: side_effect() => count++;
On 2012/09/05 08:54:54, kasperl wrote:
> sideEffect

Done.

[ahe, 2012-09-05 09:34:35]

Frontend changes: LGTM!

https://chromiumcodereview.appspot.com/10855174/diff/20023/lib/compiler/imple...
File lib/compiler/implementation/compiler.dart (right):

https://chromiumcodereview.appspot.com/10855174/diff/20023/lib/compiler/imple...
lib/compiler/implementation/compiler.dart:760: /** Compiles compile-time
constants. Must not return `null`. */
[: null :]

https://chromiumcodereview.appspot.com/10855174/diff/20023/lib/compiler/imple...
lib/compiler/implementation/compiler.dart:760: /** Compiles compile-time
constants. Must not return `null`. */
What happens if element isn't a constant?

[floitsch, 2012-09-05 10:43:16]

https://chromiumcodereview.appspot.com/10855174/diff/20023/lib/compiler/imple...
File lib/compiler/implementation/compiler.dart (right):

https://chromiumcodereview.appspot.com/10855174/diff/20023/lib/compiler/imple...
lib/compiler/implementation/compiler.dart:760: /** Compiles compile-time
constants. Must not return `null`. */
On 2012/09/05 09:34:35, ahe wrote:
> [: null :]

Done.

https://chromiumcodereview.appspot.com/10855174/diff/20023/lib/compiler/imple...
lib/compiler/implementation/compiler.dart:760: /** Compiles compile-time
constants. Must not return `null`. */
On 2012/09/05 09:34:35, ahe wrote:
> What happens if element isn't a constant?
Then an error is reported. Updated comment.