When canonicalize instances check if all fields are canonical. If a field is a non-canonical number…

When canonicalize instances check if all fields are canonical. If a field is a non-canonical number or strings, canonicalize it. Otherwise report an error if a field is not canonical.

R=hausner@google.com

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

[Ivan Posva, 2013-06-11 21:53:03]

Can you explain what you mean by missing canonicalization of closures?

Thanks,
-Ivan

[srdjan, 2013-06-11 22:26:40]

On 2013/06/11 21:53:03, Ivan Posva wrote:
> Can you explain what you mean by missing canonicalization of closures?
> 
> Thanks,
> -Ivan

They are not marked as canonical.

[srdjan, 2013-06-11 22:29:21]

On 2013/06/11 22:26:52, srdjan wrote:

Correction, they actually are... adapting code.

[hausner, 2013-06-11 23:29:24]

Almost LGTM

https://codereview.chromium.org/16780009/diff/17001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/16780009/diff/17001/runtime/vm/object.cc#newc...
runtime/vm/object.cc:53: DEFINE_FLAG(bool, report_incorrect_const_objects, true,
Why do we need the flag?

https://codereview.chromium.org/16780009/diff/17001/runtime/vm/object.cc#newc...
runtime/vm/object.cc:9106: if (cls.id() >= kNumPredefinedCids) {
As discussed offline, this excludes arrays.

https://codereview.chromium.org/16780009/diff/17001/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/16780009/diff/17001/runtime/vm/parser.cc#newc...
runtime/vm/parser.cc:8068: ErrorMsg("Invalid const object %s", error_str);
The error position is not necessarily the current token position. I would pass
in the token position as a parameter.

https://codereview.chromium.org/16780009/diff/17001/runtime/vm/parser.cc#newc...
runtime/vm/parser.cc:8157: instance ^= Instance::New(type_class, Heap::kOld);
Why ^= ?

[srdjan, 2013-06-12 00:22:22]

Thanks.

https://codereview.chromium.org/16780009/diff/17001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/16780009/diff/17001/runtime/vm/object.cc#newc...
runtime/vm/object.cc:53: DEFINE_FLAG(bool, report_incorrect_const_objects, true,
On 2013/06/11 23:29:24, hausner wrote:
> Why do we need the flag?

Removing. It is easy to disable the error reporting if issues appear.

https://codereview.chromium.org/16780009/diff/17001/runtime/vm/object.cc#newc...
runtime/vm/object.cc:9106: if (cls.id() >= kNumPredefinedCids) {
On 2013/06/11 23:29:24, hausner wrote:
> As discussed offline, this excludes arrays.

Added arrays.

https://codereview.chromium.org/16780009/diff/17001/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/16780009/diff/17001/runtime/vm/parser.cc#newc...
runtime/vm/parser.cc:8068: ErrorMsg("Invalid const object %s", error_str);
On 2013/06/11 23:29:24, hausner wrote:
> The error position is not necessarily the current token position. I would pass
> in the token position as a parameter.

Done.

https://codereview.chromium.org/16780009/diff/17001/runtime/vm/parser.cc#newc...
runtime/vm/parser.cc:8157: instance ^= Instance::New(type_class, Heap::kOld);
On 2013/06/11 23:29:24, hausner wrote:
> Why ^= ?

Weird ... removed.

[Ivan Posva, 2013-06-12 15:22:15]

https://codereview.chromium.org/16780009/diff/26001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/16780009/diff/26001/runtime/vm/object.cc#newc...
runtime/vm/object.cc:9100: if ((cls.id() >= kNumPredefinedCids) ||
cls.IsArray()) {
Why are you checking for kNumPredefinedCids here? This does not look right to
me.

[srdjan, 2013-06-12 16:04:18]

https://codereview.chromium.org/16780009/diff/26001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/16780009/diff/26001/runtime/vm/object.cc#newc...
runtime/vm/object.cc:9100: if ((cls.id() >= kNumPredefinedCids) ||
cls.IsArray()) {
On 2013/06/12 15:22:15, Ivan Posva wrote:
> Why are you checking for kNumPredefinedCids here? This does not look right to
> me.

Because I cannot iterate over content of object's that contain non-object(e.g,
RawDouble). This is the same effect as if I would iterate over fields of its
class and all its superclasses. Only instances >= kNumPredefinedCids have
fields.

[hausner, 2013-06-12 16:58:52]

LGTM.

[srdjan, 2013-06-12 21:27:28]

https://codereview.chromium.org/16780009/diff/26001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/16780009/diff/26001/runtime/vm/object.cc#newc...
runtime/vm/object.cc:9101: // Iterate over all fields, canonicalize numbers and
strings, expect all
Added a TODO(srdjan): Check that predefined classes do not have fields that need
to be canonicalized as well.

[srdjan, 2013-06-12 21:27:43]

Committed patchset #6 manually as r23934 (presubmit successful).