Serialize constant initializers.

Serialize constant initializers.

No implementation for element references in this CL.

R=paulberry@google.com
BUG=

Committed: https://github.com/dart-lang/sdk/commit/07f8831bc2c38f746ce2b327f2f3ed02e87a11cc

[Paul Berry, 2016-01-19 20:02:10]

m

https://codereview.chromium.org/1602203004/diff/1/pkg/analyzer/lib/src/summar...
File pkg/analyzer/lib/src/summary/summarize_elements.dart (right):

https://codereview.chromium.org/1602203004/diff/1/pkg/analyzer/lib/src/summar...
pkg/analyzer/lib/src/summary/summarize_elements.dart:56: * Error that described
a problem during a constant expression serialization.
s/described/describes/

https://codereview.chromium.org/1602203004/diff/1/pkg/analyzer/lib/src/summar...
pkg/analyzer/lib/src/summary/summarize_elements.dart:119: for (Token component
in expr.components) {
Nit: why not just concatenate all the components together using '.' and then
change the meaning of "makeSymbol" to convert a single string into a symbol? 
AFAICT, there's no real benefit to storing the components of the string
separately since we're just going to concatenate them using '.' at evaluation
time anyhow.

https://codereview.chromium.org/1602203004/diff/1/pkg/analyzer/lib/src/summar...
pkg/analyzer/lib/src/summary/summarize_elements.dart:130: // TODO(scheglov) how
to serialize element references?
Is this TODO still applicable?  At first glance it looks like you're doing the
right thing.

https://codereview.chromium.org/1602203004/diff/1/pkg/analyzer/lib/src/summar...
pkg/analyzer/lib/src/summary/summarize_elements.dart:141: assert(name ==
'identical');
Add a TODO noting that these assertions will fail if presented with erroneous
code.  That's not a problem for now because at the moment we're just summarizing
the SDK, but it will be in the future when we want to summarize code under
development.

https://codereview.chromium.org/1602203004/diff/1/pkg/analyzer/lib/src/summar...
pkg/analyzer/lib/src/summary/summarize_elements.dart:179: if (value >= (2 <<
32)) {
I don't think this code handles all possible ints correctly.  AFAICT it
mishandles:

- Values from 2^31 through 2^32-1 (e.g. 2^31 gets encoded as
pushInt(0x80000000), which is -2^31.  We want pushInt(0) shiftOr(0x80000000).)
- Values smaller than -2^31 (e.g. -2^31-1 gets encoded as pushInt(0x7fffffff),
which is 2^31-1.  We want pushInt(-1) shiftOr(0x7fffffff).)
- Values from 2^63 through 2^64-1 (e.g. 2^63 gets encoded as 
pushInt(0x80000000) shiftOr(0), which is -2^63.  We want pushInt(0)
shiftOr(0x80000000) shiftOr(0).)
- Values from 2^64 onward (e.g. 2^64 gets encoded as pushInt(0) shiftOr(0),
which is 0.  We want pushInt(1) shiftOr(0) shiftOr(0).  We need a while loop to
produce the shiftOrs because Dart supports arbitrary precision ints).

Note: since I suspect these values don't occur in the SDK, I would also be ok,
for now, with asserting that abs(value) < (2 << 31) and adding a TODO comment.

https://codereview.chromium.org/1602203004/diff/1/pkg/analyzer/lib/src/summar...
pkg/analyzer/lib/src/summary/summarize_elements.dart:250: // TODO(scheglov)
named arguments?
Oh yeah, good catch.  Let me know if you want to brainstorm about how to deal
with this.

https://codereview.chromium.org/1602203004/diff/1/pkg/analyzer/lib/src/summar...
pkg/analyzer/lib/src/summary/summarize_elements.dart:304:
operations.add(UnlinkedConstOperation.pushString);
I think this does the wrong thing if one or more of the component strings uses
interpolation.

https://codereview.chromium.org/1602203004/diff/1/pkg/analyzer/test/src/summa...
File pkg/analyzer/test/src/summary/summary_test.dart (right):

https://codereview.chromium.org/1602203004/diff/1/pkg/analyzer/test/src/summa...
pkg/analyzer/test/src/summary/summary_test.dart:1794: // TODO(scheglov) Not sure
for to represent a field reference
Yeah, I haven't figured out how to do this either.  Let me know if you'd like to
discuss it.

https://codereview.chromium.org/1602203004/diff/1/pkg/analyzer/test/src/summa...
pkg/analyzer/test/src/summary/summary_test.dart:3844:
references[i](constExpr.references[i]);
I think you might mean:

expect(references[i](constExpr.references[i]), isTrue);

https://codereview.chromium.org/1602203004/diff/1/pkg/analyzer/tool/summary/i...
File pkg/analyzer/tool/summary/idl.dart (right):

https://codereview.chromium.org/1602203004/diff/1/pkg/analyzer/tool/summary/i...
pkg/analyzer/tool/summary/idl.dart:459: * Pop the top n values from the stack
(where n is obtained from
See my comments at summarize_elements.dart line 119.

[scheglov, 2016-01-19 20:59:28]

PTAL

https://codereview.chromium.org/1602203004/diff/1/pkg/analyzer/lib/src/summar...
File pkg/analyzer/lib/src/summary/summarize_elements.dart (right):

https://codereview.chromium.org/1602203004/diff/1/pkg/analyzer/lib/src/summar...
pkg/analyzer/lib/src/summary/summarize_elements.dart:56: * Error that described
a problem during a constant expression serialization.
On 2016/01/19 20:02:10, Paul Berry wrote:
> s/described/describes/

Done.

https://codereview.chromium.org/1602203004/diff/1/pkg/analyzer/lib/src/summar...
pkg/analyzer/lib/src/summary/summarize_elements.dart:119: for (Token component
in expr.components) {
On 2016/01/19 20:02:10, Paul Berry wrote:
> Nit: why not just concatenate all the components together using '.' and then
> change the meaning of "makeSymbol" to convert a single string into a symbol? 
> AFAICT, there's no real benefit to storing the components of the string
> separately since we're just going to concatenate them using '.' at evaluation
> time anyhow.

Done.

https://codereview.chromium.org/1602203004/diff/1/pkg/analyzer/lib/src/summar...
pkg/analyzer/lib/src/summary/summarize_elements.dart:130: // TODO(scheglov) how
to serialize element references?
On 2016/01/19 20:02:10, Paul Berry wrote:
> Is this TODO still applicable?  At first glance it looks like you're doing the
> right thing.

Yes, I don't like how we do this.
We discussed this offline.

https://codereview.chromium.org/1602203004/diff/1/pkg/analyzer/lib/src/summar...
pkg/analyzer/lib/src/summary/summarize_elements.dart:141: assert(name ==
'identical');
On 2016/01/19 20:02:10, Paul Berry wrote:
> Add a TODO noting that these assertions will fail if presented with erroneous
> code.  That's not a problem for now because at the moment we're just
summarizing
> the SDK, but it will be in the future when we want to summarize code under
> development.

I've added checks that throw _ConstExprSerializationError.

https://codereview.chromium.org/1602203004/diff/1/pkg/analyzer/lib/src/summar...
pkg/analyzer/lib/src/summary/summarize_elements.dart:179: if (value >= (2 <<
32)) {
On 2016/01/19 20:02:10, Paul Berry wrote:
> I don't think this code handles all possible ints correctly.  AFAICT it
> mishandles:
> 
> - Values from 2^31 through 2^32-1 (e.g. 2^31 gets encoded as
> pushInt(0x80000000), which is -2^31.  We want pushInt(0) shiftOr(0x80000000).)
> - Values smaller than -2^31 (e.g. -2^31-1 gets encoded as pushInt(0x7fffffff),
> which is 2^31-1.  We want pushInt(-1) shiftOr(0x7fffffff).)
> - Values from 2^63 through 2^64-1 (e.g. 2^63 gets encoded as 
> pushInt(0x80000000) shiftOr(0), which is -2^63.  We want pushInt(0)
> shiftOr(0x80000000) shiftOr(0).)
> - Values from 2^64 onward (e.g. 2^64 gets encoded as pushInt(0) shiftOr(0),
> which is 0.  We want pushInt(1) shiftOr(0) shiftOr(0).  We need a while loop
to
> produce the shiftOrs because Dart supports arbitrary precision ints).
> 
> Note: since I suspect these values don't occur in the SDK, I would also be ok,
> for now, with asserting that abs(value) < (2 << 31) and adding a TODO comment.

Done.

https://codereview.chromium.org/1602203004/diff/1/pkg/analyzer/lib/src/summar...
pkg/analyzer/lib/src/summary/summarize_elements.dart:304:
operations.add(UnlinkedConstOperation.pushString);
On 2016/01/19 20:02:10, Paul Berry wrote:
> I think this does the wrong thing if one or more of the component strings uses
> interpolation.

Done.

https://codereview.chromium.org/1602203004/diff/1/pkg/analyzer/test/src/summa...
File pkg/analyzer/test/src/summary/summary_test.dart (right):

https://codereview.chromium.org/1602203004/diff/1/pkg/analyzer/test/src/summa...
pkg/analyzer/test/src/summary/summary_test.dart:3844:
references[i](constExpr.references[i]);
On 2016/01/19 20:02:10, Paul Berry wrote:
> I think you might mean:
> 
> expect(references[i](constExpr.references[i]), isTrue);

Nesting `expect` invocations does not work well.
So, the function does everything - validation and its own `expect` invocations.
But the name is not very good.
Renamed to `_UnlinkedTypeRefValidator`.

https://codereview.chromium.org/1602203004/diff/1/pkg/analyzer/tool/summary/i...
File pkg/analyzer/tool/summary/idl.dart (right):

https://codereview.chromium.org/1602203004/diff/1/pkg/analyzer/tool/summary/i...
pkg/analyzer/tool/summary/idl.dart:459: * Pop the top n values from the stack
(where n is obtained from
On 2016/01/19 20:02:10, Paul Berry wrote:
> See my comments at summarize_elements.dart line 119.

Done.

[Paul Berry, 2016-01-19 21:39:29]

lgtm

https://codereview.chromium.org/1602203004/diff/40001/pkg/analyzer/lib/src/ge...
File pkg/analyzer/lib/src/generated/constant.dart (right):

https://codereview.chromium.org/1602203004/diff/40001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/constant.dart:494: print('???');
Temporary debugging code I assume?  Should be removed.

[scheglov, 2016-01-19 21:42:55]

Description was changed from

==========
Serialize constant initializers.

No implementation for element references in this CL.

R=paulberry@google.com
BUG=
==========

to

==========
Serialize constant initializers.

No implementation for element references in this CL.

R=paulberry@google.com
BUG=

Committed:
https://github.com/dart-lang/sdk/commit/07f8831bc2c38f746ce2b327f2f3ed02e87a11cc
==========

[scheglov, 2016-01-19 21:42:55]

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