Use ConstExp for storing constants.

Use ConstExp for storing constants.

Main modification in

compile_time_constants.dart
cps_ir/const_expression.dart
cps_ir/cps_ir_builder.dart

R=efortuna@google.com, sigurdm@google.com

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

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

[Johnni Winther, 2014-09-16 10:44:47]

johnniwinther@google.com changed reviewers:
+ sigurdm@google.com

[sigurdm, 2014-09-17 10:29:41]

LGTM

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

https://codereview.chromium.org/574683002/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/compile_time_constants.dart:235: //
TODO(johnniwinther): Change to create [ConstExp] instead of [Constant].
I think this TODO is solved now?

https://codereview.chromium.org/574683002/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/compile_time_constants.dart:236: class
CompileTimeConstantEvaluator extends Visitor<EvaluatedConstant> {
We might want to consider evaluating the constants from ConstExp instead of
Node, or in some other way decouple creation of `Constant`s from visiting the
AST

https://codereview.chromium.org/574683002/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/compile_time_constants.dart:1047:
class EvaluatedConstant {
Document this class esp. why we need it.

Maybe it needs a rename - now a ConstExp knows its value, so it is hard to see
how it is distinguished from: "EvaluatedConstant"

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

https://codereview.chromium.org/574683002/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/constants.dart:26: abstract class
Constant {
We might consider renaming this `ConstantValue` though it is maybe too verbose.
`Constant` in itself does not say much.

https://codereview.chromium.org/574683002/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/constants.dart:115: String toSyntax()
=> value.toString();
maybe call it `unparse` or something similar. `toSyntax` indicates we have a
type `Syntax`.

https://codereview.chromium.org/574683002/diff/20001/sdk/lib/_internal/compil...
File
sdk/lib/_internal/compiler/implementation/dart_backend/backend_ast_emitter.dart
(right):

https://codereview.chromium.org/574683002/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/dart_backend/backend_ast_emitter.dart:776:
@override
Maybe add override to all the overriding methods

https://codereview.chromium.org/574683002/diff/20001/sdk/lib/_internal/compil...
File
sdk/lib/_internal/compiler/implementation/inferrer/simple_types_inferrer.dart
(right):

https://codereview.chromium.org/574683002/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/inferrer/simple_types_inferrer.dart:988:
return constant.value.value;
We talked about renaming the "value" field of primitive constants to
primitiveValue - that would make this look nicer.

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

https://codereview.chromium.org/574683002/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/js_backend/type_variable_handler.dart:86:
EvaluatedConstant constant =
Add TODO to make this less front-end specific

[Johnni Winther, 2014-09-17 12:20:40]

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

https://codereview.chromium.org/574683002/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/compile_time_constants.dart:235: //
TODO(johnniwinther): Change to create [ConstExp] instead of [Constant].
On 2014/09/17 10:29:40, sigurdm wrote:
> I think this TODO is solved now?

Done.

https://codereview.chromium.org/574683002/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/compile_time_constants.dart:236: class
CompileTimeConstantEvaluator extends Visitor<EvaluatedConstant> {
On 2014/09/17 10:29:40, sigurdm wrote:
> We might want to consider evaluating the constants from ConstExp instead of
> Node, or in some other way decouple creation of `Constant`s from visiting the
> AST

Added a TODO.

https://codereview.chromium.org/574683002/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/compile_time_constants.dart:1047:
class EvaluatedConstant {
On 2014/09/17 10:29:40, sigurdm wrote:
> Document this class esp. why we need it.
> 
> Maybe it needs a rename - now a ConstExp knows its value, so it is hard to see
> how it is distinguished from: "EvaluatedConstant" 

Done.

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

https://codereview.chromium.org/574683002/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/constants.dart:26: abstract class
Constant {
On 2014/09/17 10:29:40, sigurdm wrote:
> We might consider renaming this `ConstantValue` though it is maybe too
verbose.
> `Constant` in itself does not say much.

Added a TODO.

https://codereview.chromium.org/574683002/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/constants.dart:115: String toSyntax()
=> value.toString();
On 2014/09/17 10:29:41, sigurdm wrote:
> maybe call it `unparse` or something similar. `toSyntax` indicates we have a
> type `Syntax`.

Done.

https://codereview.chromium.org/574683002/diff/20001/sdk/lib/_internal/compil...
File
sdk/lib/_internal/compiler/implementation/dart_backend/backend_ast_emitter.dart
(right):

https://codereview.chromium.org/574683002/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/dart_backend/backend_ast_emitter.dart:776:
@override
On 2014/09/17 10:29:41, sigurdm wrote:
> Maybe add override to all the overriding methods

Done.

https://codereview.chromium.org/574683002/diff/20001/sdk/lib/_internal/compil...
File
sdk/lib/_internal/compiler/implementation/inferrer/simple_types_inferrer.dart
(right):

https://codereview.chromium.org/574683002/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/inferrer/simple_types_inferrer.dart:988:
return constant.value.value;
On 2014/09/17 10:29:41, sigurdm wrote:
> We talked about renaming the "value" field of primitive constants to
> primitiveValue - that would make this look nicer.

Added a TODO on [PrimitiveConstant.value].

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

https://codereview.chromium.org/574683002/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/js_backend/type_variable_handler.dart:86:
EvaluatedConstant constant =
On 2014/09/17 10:29:41, sigurdm wrote:
> Add TODO to make this less front-end specific

Done.

[Johnni Winther, 2014-09-17 12:40:50]

Committed patchset #3 (id:40001) manually as 40372 (presubmit successful).

[Johnni Winther, 2014-09-25 07:45:10]

Patchset #5 (id:80001) has been deleted

[Johnni Winther, 2014-09-25 07:45:57]

johnniwinther@google.com changed reviewers:
+ efortuna@google.com

[Johnni Winther, 2014-09-25 07:45:58]

sigurdm@: PTAL at the latest patch.
emily@: PTAL at the changes to docgen.

[karlklose, 2014-09-25 08:43:24]

karlklose@google.com changed reviewers:
+ karlklose@google.com

[karlklose, 2014-09-25 08:43:25]

DBC

https://codereview.chromium.org/574683002/diff/100001/sdk/lib/_internal/compi...
File sdk/lib/_internal/compiler/implementation/compiler.dart (right):

https://codereview.chromium.org/574683002/diff/100001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/compiler.dart:2113: /// Returns `true`
if [s] is private if used as an identifier.
'if' -> 'when'?

https://codereview.chromium.org/574683002/diff/100001/sdk/lib/_internal/compi...
File sdk/lib/_internal/compiler/implementation/constants.dart (right):

https://codereview.chromium.org/574683002/diff/100001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/constants.dart:64: /// This value in
Dart syntax, if possible.
'This value' -> 'the value of this constant'?

https://codereview.chromium.org/574683002/diff/100001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/constants.dart:67: /// expression from
the value so the unparse of these is best effort.
Why is there no way? Don't we have the constructors available?

https://codereview.chromium.org/574683002/diff/100001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/constants.dart:77:
assertDebugMode("Use Constant.unparse() or Constant.toStructuredString() "
A comment should be enough, I don't think we have to introduce a new mode.

https://codereview.chromium.org/574683002/diff/100001/sdk/lib/_internal/compi...
File sdk/lib/_internal/compiler/implementation/helpers/helpers.dart (right):

https://codereview.chromium.org/574683002/diff/100001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/helpers/helpers.dart:30: /// Enables
debug mode.
Why do we need this mode?

We already test that the helpers are not used in production and we only use it
for a hint that (IMHO) could do with a comment.

https://codereview.chromium.org/574683002/diff/100001/tests/compiler/dart2js/...
File tests/compiler/dart2js/backend_dart/sexpr_test.dart (right):

https://codereview.chromium.org/574683002/diff/100001/tests/compiler/dart2js/...
tests/compiler/dart2js/backend_dart/sexpr_test.dart:144: } catch (e, s) {
Why this change? Is this debug code or is the stacktrace not showing correclty
in the test setup>

[sigurdm, 2014-09-25 08:44:56]

LGTM

https://codereview.chromium.org/574683002/diff/100001/sdk/lib/_internal/compi...
File sdk/lib/_internal/compiler/implementation/helpers/helpers.dart (right):

https://codereview.chromium.org/574683002/diff/100001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/helpers/helpers.dart:86:
enableDebugMode();
Maybe ensure that debug mode is disabled after debug printing.

[Emily Fortuna, 2014-09-26 20:17:42]

lgtm

[Johnni Winther, 2014-09-29 08:24:44]

https://codereview.chromium.org/574683002/diff/100001/sdk/lib/_internal/compi...
File sdk/lib/_internal/compiler/implementation/compiler.dart (right):

https://codereview.chromium.org/574683002/diff/100001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/compiler.dart:2113: /// Returns `true`
if [s] is private if used as an identifier.
On 2014/09/25 08:43:25, karlklose wrote:
> 'if' -> 'when'?

Done.

https://codereview.chromium.org/574683002/diff/100001/sdk/lib/_internal/compi...
File sdk/lib/_internal/compiler/implementation/constants.dart (right):

https://codereview.chromium.org/574683002/diff/100001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/constants.dart:64: /// This value in
Dart syntax, if possible.
On 2014/09/25 08:43:25, karlklose wrote:
> 'This value' -> 'the value of this constant'?

Done.

https://codereview.chromium.org/574683002/diff/100001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/constants.dart:67: /// expression from
the value so the unparse of these is best effort.
On 2014/09/25 08:43:25, karlklose wrote:
> Why is there no way? Don't we have the constructors available?

Constant doesn't store the constructor only the class, ConstExp does.

https://codereview.chromium.org/574683002/diff/100001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/constants.dart:77:
assertDebugMode("Use Constant.unparse() or Constant.toStructuredString() "
On 2014/09/25 08:43:25, karlklose wrote:
> A comment should be enough, I don't think we have to introduce a new mode.

The misuse of toString for semantic purpose hides too easily and have been a
major problem in the work of this CL. The use of [assertDebugCode] is an attempt
to actively detect and avoid the situation. I'm using ConstExp and Constant as
guinea pigs to see if this is a practical approach to solving this issue. If it
works, I want to use the scheme in for instance DartType where we have a similar
issue. If it doesn't work I'll remove it (or move some functionality to
helpers). And by 'work' I mean 'solves more problems than it introduces'.

https://codereview.chromium.org/574683002/diff/100001/sdk/lib/_internal/compi...
File sdk/lib/_internal/compiler/implementation/helpers/helpers.dart (right):

https://codereview.chromium.org/574683002/diff/100001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/helpers/helpers.dart:30: /// Enables
debug mode.
On 2014/09/25 08:43:25, karlklose wrote:
> Why do we need this mode?
> 
> We already test that the helpers are not used in production and we only use it
> for a hint that (IMHO) could do with a comment.

See comment on Constant.toString()

https://codereview.chromium.org/574683002/diff/100001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/helpers/helpers.dart:86:
enableDebugMode();
On 2014/09/25 08:44:56, sigurdm wrote:
> Maybe ensure that debug mode is disabled after debug printing.

TODO added.

https://codereview.chromium.org/574683002/diff/100001/tests/compiler/dart2js/...
File tests/compiler/dart2js/backend_dart/sexpr_test.dart (right):

https://codereview.chromium.org/574683002/diff/100001/tests/compiler/dart2js/...
tests/compiler/dart2js/backend_dart/sexpr_test.dart:144: } catch (e, s) {
On 2014/09/25 08:43:25, karlklose wrote:
> Why this change? Is this debug code or is the stacktrace not showing correclty
> in the test setup>

On error the unstringifier now throws instead of asserting. This improves the
messaging in case of failures.

[Johnni Winther, 2014-09-29 09:19:59]

Committed patchset #6 (id:120001) manually as 40759 (presubmit successful).