Support serialization of the compiler backbone.

Support serialization of the compiler backbone.

BUG=
R=floitsch@google.com

Committed: https://github.com/dart-lang/sdk/commit/8aa5245b14fa9db0794c095c7247d97229b4c604

[Johnni Winther, 2015-06-25 13:48:39]

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

[floitsch, 2015-06-26 20:54:06]

FYI.

https://codereview.chromium.org/1192103002/diff/20001/pkg/compiler/lib/src/co...
File pkg/compiler/lib/src/compiler.dart (right):

https://codereview.chromium.org/1192103002/diff/20001/pkg/compiler/lib/src/co...
pkg/compiler/lib/src/compiler.dart:1745: //print('------------ $world is closed
--------------');
remove.

https://codereview.chromium.org/1192103002/diff/20001/pkg/compiler/lib/src/el...
File pkg/compiler/lib/src/elements/elements.dart (right):

https://codereview.chromium.org/1192103002/diff/20001/pkg/compiler/lib/src/el...
pkg/compiler/lib/src/elements/elements.dart:398: //bool get isResolved;
remove.

https://codereview.chromium.org/1192103002/diff/20001/pkg/compiler/lib/src/el...
pkg/compiler/lib/src/elements/elements.dart:908: * to private 'library name' for
scripts to use for instance in dartdoc.
sentence isn't English.

https://codereview.chromium.org/1192103002/diff/20001/pkg/compiler/lib/src/el...
pkg/compiler/lib/src/elements/elements.dart:910: * Note: the returned filename
will still be escaped ("a%20b.dart" instead of
is still escaped.

https://codereview.chromium.org/1192103002/diff/20001/pkg/compiler/lib/src/li...
File pkg/compiler/lib/src/library_loader.dart (right):

https://codereview.chromium.org/1192103002/diff/20001/pkg/compiler/lib/src/li...
pkg/compiler/lib/src/library_loader.dart:555: library =
compiler.serialization.readLibrary(resolvedUri);
This is confusing:
we have two readLibrary, but they don't do the same thing.
It seems like the second "readLibrary" doesn't have a good name.

https://codereview.chromium.org/1192103002/diff/20001/pkg/compiler/lib/src/re...
File pkg/compiler/lib/src/resolution/class_hierarchy.dart (right):

https://codereview.chromium.org/1192103002/diff/20001/pkg/compiler/lib/src/re...
pkg/compiler/lib/src/resolution/class_hierarchy.dart:274:
element.typeVariables.forEach((TypeVariableType type) {
Since this closure captures something I would slightly prefer if this was a `for
(TypeVariableType type in element.typeVariables)` loop.
But then I would change the next loop too (for consistency).

https://codereview.chromium.org/1192103002/diff/20001/pkg/compiler/lib/src/se...
File pkg/compiler/lib/src/serialization/constant_serialization.dart (right):

https://codereview.chromium.org/1192103002/diff/20001/pkg/compiler/lib/src/se...
pkg/compiler/lib/src/serialization/constant_serialization.dart:15: class
ConstantSerializer
Comment what this class does.

Destructures, decomposes, or simplifies a const object.
Explain how it fits into the general (de)serialization strategy.

https://codereview.chromium.org/1192103002/diff/20001/pkg/compiler/lib/src/se...
pkg/compiler/lib/src/serialization/constant_serialization.dart:19: void
apply(ConstantExpression exp, ObjectSerializer serializer) {
I prefer to call this "visit", but I have seen "apply" before (maybe from you).

https://codereview.chromium.org/1192103002/diff/20001/pkg/compiler/lib/src/se...
pkg/compiler/lib/src/serialization/constant_serialization.dart:107:
ObjectSerializer serializer) {
indentation.

https://codereview.chromium.org/1192103002/diff/20001/pkg/compiler/lib/src/se...
pkg/compiler/lib/src/serialization/constant_serialization.dart:114:
ObjectSerializer serializer) {
ditto and below.

https://codereview.chromium.org/1192103002/diff/20001/pkg/compiler/lib/src/se...
pkg/compiler/lib/src/serialization/constant_serialization.dart:184: class
ConstantDeserializer {
Ditto.

https://codereview.chromium.org/1192103002/diff/20001/pkg/compiler/lib/src/se...
pkg/compiler/lib/src/serialization/constant_serialization.dart:294:
ObjectSerializer serializer) {
indentation.

https://codereview.chromium.org/1192103002/diff/20001/pkg/compiler/lib/src/se...
pkg/compiler/lib/src/serialization/constant_serialization.dart:360: } catch (e)
{
don't use try/catch.
if it's for the 'int.parse', use the optional argument.

https://codereview.chromium.org/1192103002/diff/20001/pkg/compiler/lib/src/se...
File pkg/compiler/lib/src/serialization/element_serialization.dart (right):

https://codereview.chromium.org/1192103002/diff/20001/pkg/compiler/lib/src/se...
pkg/compiler/lib/src/serialization/element_serialization.dart:68: class
SerializerUtil {
generally we don't have utility classes (according to style guide).
I don't care.

https://codereview.chromium.org/1192103002/diff/20001/pkg/compiler/lib/src/se...
pkg/compiler/lib/src/serialization/element_serialization.dart:99: static void
serializerParameters(FunctionElement element,
serializeParameters

https://codereview.chromium.org/1192103002/diff/20001/pkg/compiler/lib/src/se...
pkg/compiler/lib/src/serialization/element_serialization.dart:151: void
addTags(LibraryElement library) {
new line before and after nested functions.

https://codereview.chromium.org/1192103002/diff/20001/pkg/compiler/lib/src/se...
pkg/compiler/lib/src/serialization/element_serialization.dart:348: if
(element.isInstanceMember) return SerializedElementKind.INSTANCE_FUNCTION;
long lines.

https://codereview.chromium.org/1192103002/diff/20001/pkg/compiler/lib/src/se...
File pkg/compiler/lib/src/serialization/json_serializer.dart (right):

https://codereview.chromium.org/1192103002/diff/20001/pkg/compiler/lib/src/se...
pkg/compiler/lib/src/serialization/json_serializer.dart:34: /// Serialiation
decoder for JSON.
Serialization

https://codereview.chromium.org/1192103002/diff/20001/pkg/compiler/lib/src/se...
File pkg/compiler/lib/src/serialization/modelz.dart (right):

https://codereview.chromium.org/1192103002/diff/20001/pkg/compiler/lib/src/se...
pkg/compiler/lib/src/serialization/modelz.dart:184: class MappedContainer {
documentation.

https://codereview.chromium.org/1192103002/diff/20001/pkg/compiler/lib/src/se...
pkg/compiler/lib/src/serialization/modelz.dart:223: class ListedContainer {
documentation.

https://codereview.chromium.org/1192103002/diff/20001/pkg/compiler/lib/src/se...
pkg/compiler/lib/src/serialization/modelz.dart:834:
//print('$this:[offsets=$offsets,types=$types,supertypes=$supertypes,levels=$levels]');
debug.

https://codereview.chromium.org/1192103002/diff/20001/pkg/compiler/lib/src/se...
File pkg/compiler/lib/src/serialization/serialization.dart (right):

https://codereview.chromium.org/1192103002/diff/20001/pkg/compiler/lib/src/se...
pkg/compiler/lib/src/serialization/serialization.dart:18: /// Abstract base
interface for [ObjectSerializer] and [MapSerializer].
Misses documentation.

https://codereview.chromium.org/1192103002/diff/20001/pkg/compiler/lib/src/se...
pkg/compiler/lib/src/serialization/serialization.dart:89: /// A serializer that
associates values with [Key]s.
Documentation.

https://codereview.chromium.org/1192103002/diff/20001/pkg/compiler/lib/src/se...
pkg/compiler/lib/src/serialization/serialization.dart:100: /// [MapSerializer]s.
this doesn't help.
Why would I have a list of ObjectSerializers and MapSerializers?
The API doesn't even reflect this.

https://codereview.chromium.org/1192103002/diff/20001/pkg/compiler/lib/src/se...
pkg/compiler/lib/src/serialization/serialization.dart:238: /// A deserializer of
a list of map values.
what "map" values?

https://codereview.chromium.org/1192103002/diff/20001/pkg/compiler/lib/src/se...
pkg/compiler/lib/src/serialization/serialization.dart:277:
_getElementValue(library);
Add comment, why we don't care for the ElementValue.

https://codereview.chromium.org/1192103002/diff/20001/pkg/compiler/lib/src/se...
pkg/compiler/lib/src/serialization/serialization.dart:307: serializer.serialize(
Ideally (especially if we decide not to use the encoder for the memory layout),
this could become:

// Delay the serialization of the object itself to avoid loops, and to keep the
// call stack small.
_pendingList.add(() {
  dataObject.setValue(serializer.serialize(element, kind));
});

https://codereview.chromium.org/1192103002/diff/20001/pkg/compiler/lib/src/se...
pkg/compiler/lib/src/serialization/serialization.dart:354: void
setConstants(Map<dynamic, Value> map,
There is no reason to be imperative here. Return the ListValue directly (or null
if empty).

Don't call this "setConstants" which is the same name as in the
AbstractSerializer.
Maybe really just "serializeConstants".

Same for setConstant, setType, ...

https://codereview.chromium.org/1192103002/diff/20001/pkg/compiler/lib/src/se...
pkg/compiler/lib/src/serialization/serialization.dart:458: final _mainData;
mainData is not a good name.

https://codereview.chromium.org/1192103002/diff/20001/pkg/compiler/lib/src/se...
pkg/compiler/lib/src/serialization/serialization.dart:705: //
TODO(johnniwinther): Split output encoding from memory object encoding.
Yes. But by removing the memory strategy. Just go for one simply IR that has
nothing to do with the encoder.
That could potentially change a lot of the other things (and definitely remove
some interfaces).

https://codereview.chromium.org/1192103002/diff/20001/pkg/compiler/lib/src/se...
File pkg/compiler/lib/src/serialization/task.dart (right):

https://codereview.chromium.org/1192103002/diff/20001/pkg/compiler/lib/src/se...
pkg/compiler/lib/src/serialization/task.dart:42: /// This will not resolved the
element but only compute the [WorldImpact].
resolve

https://codereview.chromium.org/1192103002/diff/20001/pkg/compiler/lib/src/se...
pkg/compiler/lib/src/serialization/task.dart:68: WorldImpact
getWorldImpact(Element element);
computeWorldImpact?

https://codereview.chromium.org/1192103002/diff/20001/tests/compiler/dart2js/...
File tests/compiler/dart2js/serialization_analysis_test.dart (right):

https://codereview.chromium.org/1192103002/diff/20001/tests/compiler/dart2js/...
tests/compiler/dart2js/serialization_analysis_test.dart:22: const Test(const {
empty lines between tests.
Here and in the rest of the file.

[Johnni Winther, 2015-07-03 12:13:46]

PTAL

https://codereview.chromium.org/1192103002/diff/20001/pkg/compiler/lib/src/co...
File pkg/compiler/lib/src/compiler.dart (right):

https://codereview.chromium.org/1192103002/diff/20001/pkg/compiler/lib/src/co...
pkg/compiler/lib/src/compiler.dart:1745: //print('------------ $world is closed
--------------');
On 2015/06/26 20:54:03, floitsch wrote:
> remove.

Done.

https://codereview.chromium.org/1192103002/diff/20001/pkg/compiler/lib/src/el...
File pkg/compiler/lib/src/elements/elements.dart (right):

https://codereview.chromium.org/1192103002/diff/20001/pkg/compiler/lib/src/el...
pkg/compiler/lib/src/elements/elements.dart:398: //bool get isResolved;
On 2015/06/26 20:54:04, floitsch wrote:
> remove.

Done.

https://codereview.chromium.org/1192103002/diff/20001/pkg/compiler/lib/src/el...
pkg/compiler/lib/src/elements/elements.dart:908: * to private 'library name' for
scripts to use for instance in dartdoc.
On 2015/06/26 20:54:04, floitsch wrote:
> sentence isn't English.

Done.

https://codereview.chromium.org/1192103002/diff/20001/pkg/compiler/lib/src/el...
pkg/compiler/lib/src/elements/elements.dart:910: * Note: the returned filename
will still be escaped ("a%20b.dart" instead of
On 2015/06/26 20:54:04, floitsch wrote:
> is still escaped.

Done.

https://codereview.chromium.org/1192103002/diff/20001/pkg/compiler/lib/src/li...
File pkg/compiler/lib/src/library_loader.dart (right):

https://codereview.chromium.org/1192103002/diff/20001/pkg/compiler/lib/src/li...
pkg/compiler/lib/src/library_loader.dart:555: library =
compiler.serialization.readLibrary(resolvedUri);
On 2015/06/26 20:54:04, floitsch wrote:
> This is confusing:
> we have two readLibrary, but they don't do the same thing.
> It seems like the second "readLibrary" doesn't have a good name.

Done.

https://codereview.chromium.org/1192103002/diff/20001/pkg/compiler/lib/src/re...
File pkg/compiler/lib/src/resolution/class_hierarchy.dart (right):

https://codereview.chromium.org/1192103002/diff/20001/pkg/compiler/lib/src/re...
pkg/compiler/lib/src/resolution/class_hierarchy.dart:274:
element.typeVariables.forEach((TypeVariableType type) {
On 2015/06/26 20:54:04, floitsch wrote:
> Since this closure captures something I would slightly prefer if this was a
`for
> (TypeVariableType type in element.typeVariables)` loop.
> But then I would change the next loop too (for consistency).

Done.

https://codereview.chromium.org/1192103002/diff/20001/pkg/compiler/lib/src/se...
File pkg/compiler/lib/src/serialization/constant_serialization.dart (right):

https://codereview.chromium.org/1192103002/diff/20001/pkg/compiler/lib/src/se...
pkg/compiler/lib/src/serialization/constant_serialization.dart:15: class
ConstantSerializer
On 2015/06/26 20:54:04, floitsch wrote:
> Comment what this class does.
> 
> Destructures, decomposes, or simplifies a const object.
> Explain how it fits into the general (de)serialization strategy.

Done.

https://codereview.chromium.org/1192103002/diff/20001/pkg/compiler/lib/src/se...
pkg/compiler/lib/src/serialization/constant_serialization.dart:19: void
apply(ConstantExpression exp, ObjectSerializer serializer) {
On 2015/06/26 20:54:04, floitsch wrote:
> I prefer to call this "visit", but I have seen "apply" before (maybe from
you).

Done.

https://codereview.chromium.org/1192103002/diff/20001/pkg/compiler/lib/src/se...
pkg/compiler/lib/src/serialization/constant_serialization.dart:107:
ObjectSerializer serializer) {
On 2015/06/26 20:54:04, floitsch wrote:
> indentation.

Done.

https://codereview.chromium.org/1192103002/diff/20001/pkg/compiler/lib/src/se...
pkg/compiler/lib/src/serialization/constant_serialization.dart:114:
ObjectSerializer serializer) {
On 2015/06/26 20:54:04, floitsch wrote:
> ditto and below.

Done.

https://codereview.chromium.org/1192103002/diff/20001/pkg/compiler/lib/src/se...
pkg/compiler/lib/src/serialization/constant_serialization.dart:184: class
ConstantDeserializer {
On 2015/06/26 20:54:04, floitsch wrote:
> Ditto.

Done.

https://codereview.chromium.org/1192103002/diff/20001/pkg/compiler/lib/src/se...
pkg/compiler/lib/src/serialization/constant_serialization.dart:294:
ObjectSerializer serializer) {
On 2015/06/26 20:54:04, floitsch wrote:
> indentation.

Done.

https://codereview.chromium.org/1192103002/diff/20001/pkg/compiler/lib/src/se...
pkg/compiler/lib/src/serialization/constant_serialization.dart:360: } catch (e)
{
On 2015/06/26 20:54:04, floitsch wrote:
> don't use try/catch.
> if it's for the 'int.parse', use the optional argument.

Done.

https://codereview.chromium.org/1192103002/diff/20001/pkg/compiler/lib/src/se...
File pkg/compiler/lib/src/serialization/element_serialization.dart (right):

https://codereview.chromium.org/1192103002/diff/20001/pkg/compiler/lib/src/se...
pkg/compiler/lib/src/serialization/element_serialization.dart:68: class
SerializerUtil {
On 2015/06/26 20:54:04, floitsch wrote:
> generally we don't have utility classes (according to style guide).
> I don't care.

I don't like the mixing of public classes and top level fields/functions in a
library since import of such a library will contaminate the import scope of
importing library with names that potentially shadow instance members. We
already have the problem with 'unparse' from 'unparser.dart' shadowing
'ConstantValue.unparse' (if not for the then added hide clause).

https://codereview.chromium.org/1192103002/diff/20001/pkg/compiler/lib/src/se...
pkg/compiler/lib/src/serialization/element_serialization.dart:99: static void
serializerParameters(FunctionElement element,
On 2015/06/26 20:54:04, floitsch wrote:
> serializeParameters

Done.

https://codereview.chromium.org/1192103002/diff/20001/pkg/compiler/lib/src/se...
pkg/compiler/lib/src/serialization/element_serialization.dart:151: void
addTags(LibraryElement library) {
On 2015/06/26 20:54:04, floitsch wrote:
> new line before and after nested functions.

Done.

https://codereview.chromium.org/1192103002/diff/20001/pkg/compiler/lib/src/se...
pkg/compiler/lib/src/serialization/element_serialization.dart:348: if
(element.isInstanceMember) return SerializedElementKind.INSTANCE_FUNCTION;
On 2015/06/26 20:54:04, floitsch wrote:
> long lines.

Done.

https://codereview.chromium.org/1192103002/diff/20001/pkg/compiler/lib/src/se...
File pkg/compiler/lib/src/serialization/json_serializer.dart (right):

https://codereview.chromium.org/1192103002/diff/20001/pkg/compiler/lib/src/se...
pkg/compiler/lib/src/serialization/json_serializer.dart:34: /// Serialiation
decoder for JSON.
On 2015/06/26 20:54:04, floitsch wrote:
> Serialization

Done.

https://codereview.chromium.org/1192103002/diff/20001/pkg/compiler/lib/src/se...
File pkg/compiler/lib/src/serialization/modelz.dart (right):

https://codereview.chromium.org/1192103002/diff/20001/pkg/compiler/lib/src/se...
pkg/compiler/lib/src/serialization/modelz.dart:184: class MappedContainer {
On 2015/06/26 20:54:05, floitsch wrote:
> documentation.

Done.

https://codereview.chromium.org/1192103002/diff/20001/pkg/compiler/lib/src/se...
pkg/compiler/lib/src/serialization/modelz.dart:223: class ListedContainer {
On 2015/06/26 20:54:05, floitsch wrote:
> documentation.

Done.

https://codereview.chromium.org/1192103002/diff/20001/pkg/compiler/lib/src/se...
pkg/compiler/lib/src/serialization/modelz.dart:834:
//print('$this:[offsets=$offsets,types=$types,supertypes=$supertypes,levels=$levels]');
On 2015/06/26 20:54:05, floitsch wrote:
> debug.

Done.

https://codereview.chromium.org/1192103002/diff/20001/pkg/compiler/lib/src/se...
File pkg/compiler/lib/src/serialization/serialization.dart (right):

https://codereview.chromium.org/1192103002/diff/20001/pkg/compiler/lib/src/se...
pkg/compiler/lib/src/serialization/serialization.dart:18: /// Abstract base
interface for [ObjectSerializer] and [MapSerializer].
On 2015/06/26 20:54:05, floitsch wrote:
> Misses documentation.

Done.

https://codereview.chromium.org/1192103002/diff/20001/pkg/compiler/lib/src/se...
pkg/compiler/lib/src/serialization/serialization.dart:89: /// A serializer that
associates values with [Key]s.
On 2015/06/26 20:54:05, floitsch wrote:
> Documentation.

Done.

https://codereview.chromium.org/1192103002/diff/20001/pkg/compiler/lib/src/se...
pkg/compiler/lib/src/serialization/serialization.dart:100: /// [MapSerializer]s.
On 2015/06/26 20:54:05, floitsch wrote:
> this doesn't help.
> Why would I have a list of ObjectSerializers and MapSerializers?
> The API doesn't even reflect this.

Done.

https://codereview.chromium.org/1192103002/diff/20001/pkg/compiler/lib/src/se...
pkg/compiler/lib/src/serialization/serialization.dart:238: /// A deserializer of
a list of map values.
On 2015/06/26 20:54:05, floitsch wrote:
> what "map" values?

Done.

https://codereview.chromium.org/1192103002/diff/20001/pkg/compiler/lib/src/se...
pkg/compiler/lib/src/serialization/serialization.dart:277:
_getElementValue(library);
On 2015/06/26 20:54:05, floitsch wrote:
> Add comment, why we don't care for the ElementValue.

Done.

https://codereview.chromium.org/1192103002/diff/20001/pkg/compiler/lib/src/se...
pkg/compiler/lib/src/serialization/serialization.dart:307: serializer.serialize(
On 2015/06/26 20:54:05, floitsch wrote:
> Ideally (especially if we decide not to use the encoder for the memory
layout),
> this could become:
> 
> // Delay the serialization of the object itself to avoid loops, and to keep
the
> // call stack small.
> _pendingList.add(() {
>   dataObject.setValue(serializer.serialize(element, kind));
> });

You would need to pass the [Serializer] around then.

https://codereview.chromium.org/1192103002/diff/20001/pkg/compiler/lib/src/se...
pkg/compiler/lib/src/serialization/serialization.dart:354: void
setConstants(Map<dynamic, Value> map,
On 2015/06/26 20:54:05, floitsch wrote:
> There is no reason to be imperative here. Return the ListValue directly (or
null
> if empty).
> 
> Don't call this "setConstants" which is the same name as in the
> AbstractSerializer.
> Maybe really just "serializeConstants".
> 
> Same for setConstant, setType, ...

Done.

https://codereview.chromium.org/1192103002/diff/20001/pkg/compiler/lib/src/se...
pkg/compiler/lib/src/serialization/serialization.dart:458: final _mainData;
On 2015/06/26 20:54:05, floitsch wrote:
> mainData is not a good name.

'mainData' -> 'headerObject'.

https://codereview.chromium.org/1192103002/diff/20001/pkg/compiler/lib/src/se...
pkg/compiler/lib/src/serialization/serialization.dart:705: //
TODO(johnniwinther): Split output encoding from memory object encoding.
On 2015/06/26 20:54:05, floitsch wrote:
> Yes. But by removing the memory strategy. Just go for one simply IR that has
> nothing to do with the encoder.
> That could potentially change a lot of the other things (and definitely remove
> some interfaces).

Done.

https://codereview.chromium.org/1192103002/diff/20001/pkg/compiler/lib/src/se...
File pkg/compiler/lib/src/serialization/task.dart (right):

https://codereview.chromium.org/1192103002/diff/20001/pkg/compiler/lib/src/se...
pkg/compiler/lib/src/serialization/task.dart:42: /// This will not resolved the
element but only compute the [WorldImpact].
On 2015/06/26 20:54:06, floitsch wrote:
> resolve

Done.

https://codereview.chromium.org/1192103002/diff/20001/pkg/compiler/lib/src/se...
pkg/compiler/lib/src/serialization/task.dart:68: WorldImpact
getWorldImpact(Element element);
On 2015/06/26 20:54:05, floitsch wrote:
> computeWorldImpact?

Done.

https://codereview.chromium.org/1192103002/diff/20001/tests/compiler/dart2js/...
File tests/compiler/dart2js/serialization_analysis_test.dart (right):

https://codereview.chromium.org/1192103002/diff/20001/tests/compiler/dart2js/...
tests/compiler/dart2js/serialization_analysis_test.dart:22: const Test(const {
On 2015/06/26 20:54:06, floitsch wrote:
> empty lines between tests.
> Here and in the rest of the file.

Done.

[floitsch, 2015-07-03 17:06:26]

LGTM.
much better. thanks.

https://codereview.chromium.org/1192103002/diff/40001/pkg/compiler/lib/src/el...
File pkg/compiler/lib/src/elements/elements.dart (right):

https://codereview.chromium.org/1192103002/diff/40001/pkg/compiler/lib/src/el...
pkg/compiler/lib/src/elements/elements.dart:1500: /// The index of this type
variable within its type declaraiton.
declaration

https://codereview.chromium.org/1192103002/diff/40001/pkg/compiler/lib/src/se...
File pkg/compiler/lib/src/serialization/element_serialization.dart (right):

https://codereview.chromium.org/1192103002/diff/40001/pkg/compiler/lib/src/se...
pkg/compiler/lib/src/serialization/element_serialization.dart:112: /// Returns a
function that addeds the underlying declared elements for a
adds

https://codereview.chromium.org/1192103002/diff/40001/pkg/compiler/lib/src/se...
pkg/compiler/lib/src/serialization/element_serialization.dart:172:
addTags(element);
Since 'addTags' is not reused, why not just inline it?

https://codereview.chromium.org/1192103002/diff/40001/pkg/compiler/lib/src/se...
File pkg/compiler/lib/src/serialization/modelz.dart (right):

https://codereview.chromium.org/1192103002/diff/40001/pkg/compiler/lib/src/se...
pkg/compiler/lib/src/serialization/modelz.dart:197: Map<String, Element>
_lookupMap = {};
This is just a cache. right?
Either add a comment or rename.

https://codereview.chromium.org/1192103002/diff/40001/pkg/compiler/lib/src/se...
pkg/compiler/lib/src/serialization/modelz.dart:256: _lookupMap[name] = element;
The _lookupMap[name] will be overwritten below in line 266. Why adding it here?

same for setter.

https://codereview.chromium.org/1192103002/diff/40001/pkg/compiler/lib/src/se...
File pkg/compiler/lib/src/serialization/serialization.dart (right):

https://codereview.chromium.org/1192103002/diff/40001/pkg/compiler/lib/src/se...
pkg/compiler/lib/src/serialization/serialization.dart:143: _map[key] = new
ListValue(types.map(_serializer.createTypeValue).toList());
long line.

https://codereview.chromium.org/1192103002/diff/40001/pkg/compiler/lib/src/se...
pkg/compiler/lib/src/serialization/serialization.dart:623:
_getElementDataObject(library);
Mention that we don't care for the value, but do this call for its side-effect.

https://codereview.chromium.org/1192103002/diff/40001/pkg/compiler/lib/src/se...
pkg/compiler/lib/src/serialization/serialization.dart:643: for
(ElementSerializer serializer in ELEMENT_SERIALIZERS) {
Add comment:  Sequentially run through all serializers to find the one that
deals with the element.

[Johnni Winther, 2015-07-06 08:33:22]

https://codereview.chromium.org/1192103002/diff/40001/pkg/compiler/lib/src/el...
File pkg/compiler/lib/src/elements/elements.dart (right):

https://codereview.chromium.org/1192103002/diff/40001/pkg/compiler/lib/src/el...
pkg/compiler/lib/src/elements/elements.dart:1500: /// The index of this type
variable within its type declaraiton.
On 2015/07/03 17:06:25, floitsch wrote:
> declaration

Done.

https://codereview.chromium.org/1192103002/diff/40001/pkg/compiler/lib/src/se...
File pkg/compiler/lib/src/serialization/element_serialization.dart (right):

https://codereview.chromium.org/1192103002/diff/40001/pkg/compiler/lib/src/se...
pkg/compiler/lib/src/serialization/element_serialization.dart:112: /// Returns a
function that addeds the underlying declared elements for a
On 2015/07/03 17:06:25, floitsch wrote:
> adds

Done.

https://codereview.chromium.org/1192103002/diff/40001/pkg/compiler/lib/src/se...
pkg/compiler/lib/src/serialization/element_serialization.dart:172:
addTags(element);
On 2015/07/03 17:06:25, floitsch wrote:
> Since 'addTags' is not reused, why not just inline it?

Done.

https://codereview.chromium.org/1192103002/diff/40001/pkg/compiler/lib/src/se...
File pkg/compiler/lib/src/serialization/modelz.dart (right):

https://codereview.chromium.org/1192103002/diff/40001/pkg/compiler/lib/src/se...
pkg/compiler/lib/src/serialization/modelz.dart:197: Map<String, Element>
_lookupMap = {};
On 2015/07/03 17:06:25, floitsch wrote:
> This is just a cache. right?
> Either add a comment or rename.

Done.

https://codereview.chromium.org/1192103002/diff/40001/pkg/compiler/lib/src/se...
pkg/compiler/lib/src/serialization/modelz.dart:256: _lookupMap[name] = element;
On 2015/07/03 17:06:25, floitsch wrote:
> The _lookupMap[name] will be overwritten below in line 266. Why adding it
here?
> 
> same for setter.

Added a comment; [element] is inserted here to ensure insert order of [name].

https://codereview.chromium.org/1192103002/diff/40001/pkg/compiler/lib/src/se...
File pkg/compiler/lib/src/serialization/serialization.dart (right):

https://codereview.chromium.org/1192103002/diff/40001/pkg/compiler/lib/src/se...
pkg/compiler/lib/src/serialization/serialization.dart:143: _map[key] = new
ListValue(types.map(_serializer.createTypeValue).toList());
On 2015/07/03 17:06:25, floitsch wrote:
> long line.

Done.

https://codereview.chromium.org/1192103002/diff/40001/pkg/compiler/lib/src/se...
pkg/compiler/lib/src/serialization/serialization.dart:623:
_getElementDataObject(library);
On 2015/07/03 17:06:25, floitsch wrote:
> Mention that we don't care for the value, but do this call for its
side-effect.

Done.

https://codereview.chromium.org/1192103002/diff/40001/pkg/compiler/lib/src/se...
pkg/compiler/lib/src/serialization/serialization.dart:643: for
(ElementSerializer serializer in ELEMENT_SERIALIZERS) {
On 2015/07/03 17:06:25, floitsch wrote:
> Add comment:  Sequentially run through all serializers to find the one that
> deals with the element.

Done.

[Johnni Winther, 2015-07-06 09:31:01]

Committed patchset #6 (id:60002) manually as
8aa5245b14fa9db0794c095c7247d97229b4c604 (presubmit successful).