Compute next field offset correctly.

Compute next field offset correctly.

R=iposva@google.com

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

[Ivan Posva, 2013-11-02 00:25:55]

First round of comments.

-Ivan

https://codereview.chromium.org/56033002/diff/360001/runtime/vm/object.h
File runtime/vm/object.h (right):

https://codereview.chromium.org/56033002/diff/360001/runtime/vm/object.h#newc...
runtime/vm/object.h:533: return -1;
How about -kWordSize and then the scaling becomes automatic.

https://codereview.chromium.org/56033002/diff/360001/runtime/vm/object.h#newc...
runtime/vm/object.h:656: if (raw_ptr()->next_field_offset_in_words_ == -1) {
I think we can remove this test and just return a negative value below.

https://codereview.chromium.org/56033002/diff/360001/runtime/vm/object.h#newc...
runtime/vm/object.h:662: if (value_in_bytes == -1) {
I would think this test can go too.

https://codereview.chromium.org/56033002/diff/360001/runtime/vm/object.h#newc...
runtime/vm/object.h:1178: }
Empty line here and other places.

https://codereview.chromium.org/56033002/diff/360001/runtime/vm/object.h#newc...
runtime/vm/object.h:4331: }
Doesn't this correspond to the Dart class Type, which can indeed be extended by
Dart code. The resulting object is not very useful, but there is no provision in
the spec to prevent extension of this.

[siva, 2013-11-02 03:44:59]

https://codereview.chromium.org/56033002/diff/360001/runtime/vm/object.h
File runtime/vm/object.h (right):

https://codereview.chromium.org/56033002/diff/360001/runtime/vm/object.h#newc...
runtime/vm/object.h:533: return -1;
On 2013/11/02 00:25:55, Ivan Posva wrote:
> How about -kWordSize and then the scaling becomes automatic.

Done.

https://codereview.chromium.org/56033002/diff/360001/runtime/vm/object.h#newc...
runtime/vm/object.h:656: if (raw_ptr()->next_field_offset_in_words_ == -1) {
On 2013/11/02 00:25:55, Ivan Posva wrote:
> I think we can remove this test and just return a negative value below.

Done.

https://codereview.chromium.org/56033002/diff/360001/runtime/vm/object.h#newc...
runtime/vm/object.h:662: if (value_in_bytes == -1) {
On 2013/11/02 00:25:55, Ivan Posva wrote:
> I would think this test can go too.

Done.

https://codereview.chromium.org/56033002/diff/360001/runtime/vm/object.h#newc...
runtime/vm/object.h:1178: }
On 2013/11/02 00:25:55, Ivan Posva wrote:
> Empty line here and other places.

Done.

https://codereview.chromium.org/56033002/diff/360001/runtime/vm/object.h#newc...
runtime/vm/object.h:4331: }
On 2013/11/02 00:25:55, Ivan Posva wrote:
> Doesn't this correspond to the Dart class Type, which can indeed be extended
by
> Dart code. The resulting object is not very useful, but there is no provision
in
> the spec to prevent extension of this.

Done.

[Ivan Posva, 2013-11-03 16:08:50]

LGTM

-Ivan

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

https://codereview.chromium.org/56033002/diff/520001/runtime/vm/object.cc#new...
runtime/vm/object.cc:2556: result.set_next_field_offset(-1);
-kWordSize

https://codereview.chromium.org/56033002/diff/520001/runtime/vm/object.h
File runtime/vm/object.h (right):

https://codereview.chromium.org/56033002/diff/520001/runtime/vm/object.h#newc...
runtime/vm/object.h:1169: static intptr_t NextFieldOffset() {
We should maybe roll this into FINAL_HEAP_OBJECT_IMPLEMENTATION as it avoids
repeating it everywhere unnecessarily.

https://codereview.chromium.org/56033002/diff/520001/runtime/vm/object.h#newc...
runtime/vm/object.h:5811: return sizeof(RawGrowableObjectArray);
Are you sure we need to be able to extend the VM's implementation of growable
lists?

https://codereview.chromium.org/56033002/diff/520001/runtime/vm/object.h#newc...
runtime/vm/object.h:6460: return sizeof(RawWeakProperty);
ditto. Internal VM implementation class, should this really be extensible by
Dart user code?

https://codereview.chromium.org/56033002/diff/520001/runtime/vm/snapshot.cc
File runtime/vm/snapshot.cc (right):

https://codereview.chromium.org/56033002/diff/520001/runtime/vm/snapshot.cc#n...
runtime/vm/snapshot.cc:796: intptr_t offset = Instance::NextFieldOffset();
Please add a comment that Instance::NextFieldOffset() is the first field in the
Dart object.

[siva, 2013-11-04 16:05:00]

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

https://codereview.chromium.org/56033002/diff/520001/runtime/vm/object.cc#new...
runtime/vm/object.cc:2556: result.set_next_field_offset(-1);
On 2013/11/03 16:08:51, Ivan Posva wrote:
> -kWordSize

Done.

https://codereview.chromium.org/56033002/diff/520001/runtime/vm/object.h
File runtime/vm/object.h (right):

https://codereview.chromium.org/56033002/diff/520001/runtime/vm/object.h#newc...
runtime/vm/object.h:1169: static intptr_t NextFieldOffset() {
Rolled it into FINAL_HEAP_OBJECT_IMPLEMENTATION and this means
class Type and class WeakProperty also have negative next_field_offset which
means they cannot be extended.

On 2013/11/03 16:08:51, Ivan Posva wrote:
> We should maybe roll this into FINAL_HEAP_OBJECT_IMPLEMENTATION as it avoids
> repeating it everywhere unnecessarily.

https://codereview.chromium.org/56033002/diff/520001/runtime/vm/object.h#newc...
runtime/vm/object.h:5811: return sizeof(RawGrowableObjectArray);
Removed this.

On 2013/11/03 16:08:51, Ivan Posva wrote:
> Are you sure we need to be able to extend the VM's implementation of growable
> lists?

https://codereview.chromium.org/56033002/diff/520001/runtime/vm/object.h#newc...
runtime/vm/object.h:6460: return sizeof(RawWeakProperty);
Rolled this into FINAL_HEAL_OBJECT_IMPLEMENTATION so this would also not be
extensible.

On 2013/11/03 16:08:51, Ivan Posva wrote:
> ditto. Internal VM implementation class, should this really be extensible by
> Dart user code?

https://codereview.chromium.org/56033002/diff/520001/runtime/vm/snapshot.cc
File runtime/vm/snapshot.cc (right):

https://codereview.chromium.org/56033002/diff/520001/runtime/vm/snapshot.cc#n...
runtime/vm/snapshot.cc:796: intptr_t offset = Instance::NextFieldOffset();
On 2013/11/03 16:08:51, Ivan Posva wrote:
> Please add a comment that Instance::NextFieldOffset() is the first field in
the
> Dart object.

Done.

[siva, 2013-11-04 16:51:57]

Committed patchset #9 manually as r29818 (presubmit successful).