Compute frontend/backend specific constants.

Compute frontend/backend specific constants.

BUG=
R=floitsch@google.com, karlklose@google.com

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

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

[karlklose, 2014-04-02 13:04:44]

LGTM.

https://codereview.chromium.org/221873002/diff/1/sdk/lib/_internal/compiler/i...
File sdk/lib/_internal/compiler/implementation/compile_time_constants.dart
(right):

https://codereview.chromium.org/221873002/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/compile_time_constants.dart:18:
Constant compileNode(Node node, TreeElements elements);
Please ad comments to these methods, too. And perhaps some documentation on the
class.

https://codereview.chromium.org/221873002/diff/1/sdk/lib/_internal/compiler/i...
File sdk/lib/_internal/compiler/implementation/js_backend/backend.dart (right):

https://codereview.chromium.org/221873002/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:1154:
JavaScriptBackend backend = compiler.backend;
Isn't this the JavaScriptBackend?

https://codereview.chromium.org/221873002/diff/1/sdk/lib/_internal/compiler/i...
File
sdk/lib/_internal/compiler/implementation/js_backend/constant_handler_javascript.dart
(right):

https://codereview.chromium.org/221873002/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/js_backend/constant_handler_javascript.dart:104:
&& !element.isInstanceMember()
This is not your code, but maybe break after operators.

[floitsch, 2014-04-02 16:50:01]

Didn't go completely through every line, but afaict things look correct.
However, the names and structure is too confusing. I would be happy to
brain-storm with you and see if we can come up with better names (or different
structure).

https://codereview.chromium.org/221873002/diff/1/sdk/lib/_internal/compiler/i...
File sdk/lib/_internal/compiler/implementation/compile_time_constants.dart
(right):

https://codereview.chromium.org/221873002/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/compile_time_constants.dart:7:
abstract class ConstantHandler {
The name "ConstantHandler" is now used with three different interfaces:
ConstantHandler: getConstantForVariable
BackendConstantHandler: compileVariable, getConstantFor{Variable|Node|Metadata}
DartConstantHandler: BackendConstantHandler + compile{Constant|Node|Metadata}

They are related (hierarchically), but it's still very confusing.
Especially because some ConstantHandler subclasses are not
BackendConstantHandlers.

Could we come up with different names?

For example (and I would love better names)
ConstantHandler -> VariableToConstantMapper
BackendConstantHandler -> ConstantHandler
DartConstantHandler -> DartConstantHandler

https://codereview.chromium.org/221873002/diff/1/sdk/lib/_internal/compiler/i...
File sdk/lib/_internal/compiler/implementation/compiler.dart (right):

https://codereview.chromium.org/221873002/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/compiler.dart:78:
Backend(this.compiler);
Add comments what and when they are used.
Are they always doing backend-constant handling?
Do they also do Dart-handling?
May they ever expose non-Dart handling?

https://codereview.chromium.org/221873002/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/compiler.dart:680: constantHandler =
backend.constantCompiler,
It feels wrong (when reading) to assign a compiler to a handler. Especially when
the backend also has a constantHandler.

https://codereview.chromium.org/221873002/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/compiler.dart:940:
resolver.constantCompiler.compileConstant(coreLibrary.find('proxy'));
Why go through the resolver?

https://codereview.chromium.org/221873002/diff/1/sdk/lib/_internal/compiler/i...
File sdk/lib/_internal/compiler/implementation/deferred_load.dart (right):

https://codereview.chromium.org/221873002/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/deferred_load.dart:298:
compiler.backend.constantHandler.getConstantForNode(n, elements));
Introduce backend getter in this class?

https://codereview.chromium.org/221873002/diff/1/sdk/lib/_internal/compiler/i...
File sdk/lib/_internal/compiler/implementation/elements/elements.dart (right):

https://codereview.chromium.org/221873002/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/elements/elements.dart:1043: /// The
front-end constant of this metadata annotation.
This feels dangerous (in the sense that I can see people use it in the backend),
but I vote for keeping it the way it is now.

https://codereview.chromium.org/221873002/diff/1/sdk/lib/_internal/compiler/i...
File
sdk/lib/_internal/compiler/implementation/js_backend/constant_handler_javascript.dart
(right):

https://codereview.chromium.org/221873002/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/js_backend/constant_handler_javascript.dart:7:
class JavaScriptConstantHandler extends CompilerTask
Since this is "just" a compiler would it make sense to call it
JavaScriptConstantCompiler?

https://codereview.chromium.org/221873002/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/js_backend/constant_handler_javascript.dart:10:
JavaScriptBackendConstantHandler jsConstantCompiler;
And this is really a ConstantHandler.

[kevmoo, 2014-04-02 18:07:49]

DBC

https://codereview.chromium.org/221873002/diff/1/tests/language/const_constru...
File tests/language/const_constructor3_test.dart (right):

https://codereview.chromium.org/221873002/diff/1/tests/language/const_constru...
tests/language/const_constructor3_test.dart:15: const d = const C(0); /// 02:
static type warning, checked mode compile-time error
long line

[floitsch, 2014-04-02 18:14:59]

https://codereview.chromium.org/221873002/diff/1/tests/language/const_constru...
File tests/language/const_constructor3_test.dart (right):

https://codereview.chromium.org/221873002/diff/1/tests/language/const_constru...
tests/language/const_constructor3_test.dart:15: const d = const C(0); /// 02:
static type warning, checked mode compile-time error
On 2014/04/02 18:07:50, kevmoo wrote:
> long line

long lines are ok for negative tests. There is no way to split them.

[kevmoo, 2014-04-02 18:19:36]

https://codereview.chromium.org/221873002/diff/1/tests/language/const_constru...
File tests/language/const_constructor3_test.dart (right):

https://codereview.chromium.org/221873002/diff/1/tests/language/const_constru...
tests/language/const_constructor3_test.dart:15: const d = const C(0); /// 02:
static type warning, checked mode compile-time error
On 2014/04/02 18:14:59, floitsch wrote:
> On 2014/04/02 18:07:50, kevmoo wrote:
> > long line
> 
> long lines are ok for negative tests. There is no way to split them.

...and the PM learns something new. :-)

[Johnni Winther, 2014-04-07 11:42:26]

PTAL

https://codereview.chromium.org/221873002/diff/1/sdk/lib/_internal/compiler/i...
File sdk/lib/_internal/compiler/implementation/compile_time_constants.dart
(right):

https://codereview.chromium.org/221873002/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/compile_time_constants.dart:7:
abstract class ConstantHandler {
On 2014/04/02 16:50:02, floitsch wrote:
> The name "ConstantHandler" is now used with three different interfaces:
> ConstantHandler: getConstantForVariable
> BackendConstantHandler: compileVariable,
getConstantFor{Variable|Node|Metadata}
> DartConstantHandler: BackendConstantHandler + compile{Constant|Node|Metadata}
> 
> They are related (hierarchically), but it's still very confusing.
> Especially because some ConstantHandler subclasses are not
> BackendConstantHandlers.
> 
> Could we come up with different names?
> 
> For example (and I would love better names)
> ConstantHandler -> VariableToConstantMapper
> BackendConstantHandler -> ConstantHandler
> DartConstantHandler -> DartConstantHandler

Done.

https://codereview.chromium.org/221873002/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/compile_time_constants.dart:18:
Constant compileNode(Node node, TreeElements elements);
On 2014/04/02 13:04:44, karlklose wrote:
> Please ad comments to these methods, too. And perhaps some documentation on
the
> class.

Done.

https://codereview.chromium.org/221873002/diff/1/sdk/lib/_internal/compiler/i...
File sdk/lib/_internal/compiler/implementation/compiler.dart (right):

https://codereview.chromium.org/221873002/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/compiler.dart:78:
Backend(this.compiler);
On 2014/04/02 16:50:02, floitsch wrote:
> Add comments what and when they are used.
> Are they always doing backend-constant handling?
> Do they also do Dart-handling?
> May they ever expose non-Dart handling?

Done.

https://codereview.chromium.org/221873002/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/compiler.dart:680: constantHandler =
backend.constantCompiler,
On 2014/04/02 16:50:02, floitsch wrote:
> It feels wrong (when reading) to assign a compiler to a handler. Especially
when
> the backend also has a constantHandler.

Done.

https://codereview.chromium.org/221873002/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/compiler.dart:940:
resolver.constantCompiler.compileConstant(coreLibrary.find('proxy'));
On 2014/04/02 16:50:02, floitsch wrote:
> Why go through the resolver?

We need the frontend interpretation. Putting the constant compiler on the
resolver instead of on the Compiler itself is deliberate, to show that
compilation of compile-time constants is part of the resolution phase.

https://codereview.chromium.org/221873002/diff/1/sdk/lib/_internal/compiler/i...
File sdk/lib/_internal/compiler/implementation/deferred_load.dart (right):

https://codereview.chromium.org/221873002/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/deferred_load.dart:298:
compiler.backend.constantHandler.getConstantForNode(n, elements));
On 2014/04/02 16:50:02, floitsch wrote:
> Introduce backend getter in this class?

Done.

https://codereview.chromium.org/221873002/diff/1/sdk/lib/_internal/compiler/i...
File sdk/lib/_internal/compiler/implementation/elements/elements.dart (right):

https://codereview.chromium.org/221873002/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/elements/elements.dart:1043: /// The
front-end constant of this metadata annotation.
On 2014/04/02 16:50:02, floitsch wrote:
> This feels dangerous (in the sense that I can see people use it in the
backend),
> but I vote for keeping it the way it is now.

Maybe rename to [frontendValue] ?

https://codereview.chromium.org/221873002/diff/1/sdk/lib/_internal/compiler/i...
File sdk/lib/_internal/compiler/implementation/js_backend/backend.dart (right):

https://codereview.chromium.org/221873002/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:1154:
JavaScriptBackend backend = compiler.backend;
On 2014/04/02 13:04:44, karlklose wrote:
> Isn't this the JavaScriptBackend?

Yes! Removed.

https://codereview.chromium.org/221873002/diff/1/sdk/lib/_internal/compiler/i...
File
sdk/lib/_internal/compiler/implementation/js_backend/constant_handler_javascript.dart
(right):

https://codereview.chromium.org/221873002/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/js_backend/constant_handler_javascript.dart:7:
class JavaScriptConstantHandler extends CompilerTask
On 2014/04/02 16:50:02, floitsch wrote:
> Since this is "just" a compiler would it make sense to call it
> JavaScriptConstantCompiler?

Done.

https://codereview.chromium.org/221873002/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/js_backend/constant_handler_javascript.dart:10:
JavaScriptBackendConstantHandler jsConstantCompiler;
On 2014/04/02 16:50:02, floitsch wrote:
> And this is really a ConstantHandler.

Renamed.

https://codereview.chromium.org/221873002/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/js_backend/constant_handler_javascript.dart:104:
&& !element.isInstanceMember()
On 2014/04/02 13:04:44, karlklose wrote:
> This is not your code, but maybe break after operators.

Done.

[floitsch, 2014-04-07 12:21:41]

LGTM.

https://codereview.chromium.org/221873002/diff/1/sdk/lib/_internal/compiler/i...
File sdk/lib/_internal/compiler/implementation/elements/elements.dart (right):

https://codereview.chromium.org/221873002/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/elements/elements.dart:1043: /// The
front-end constant of this metadata annotation.
On 2014/04/07 11:42:27, Johnni Winther wrote:
> On 2014/04/02 16:50:02, floitsch wrote:
> > This feels dangerous (in the sense that I can see people use it in the
> backend),
> > but I vote for keeping it the way it is now.
> 
> Maybe rename to [frontendValue] ?

up to you. I would LGTM both.

https://codereview.chromium.org/221873002/diff/20001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/js_backend/backend.dart (right):

https://codereview.chromium.org/221873002/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:310:
JavaScriptConstantCompiler get constants {
Add comment that these are now the JS constants.

[Johnni Winther, 2014-04-08 07:09:56]

https://codereview.chromium.org/221873002/diff/20001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/js_backend/backend.dart (right):

https://codereview.chromium.org/221873002/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:310:
JavaScriptConstantCompiler get constants {
On 2014/04/07 12:21:42, floitsch wrote:
> Add comment that these are now the JS constants.

Done.

[Johnni Winther, 2014-04-08 07:13:21]

Committed patchset #2 manually as r34811 (presubmit successful).

[Johnni Winther, 2014-04-09 07:42:46]

Committed patchset #3 manually as r34862 (presubmit successful).