898623 - [binary data] Root StructType FieldInfo jsids and type objects.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

FieldInfo's are stored on the heap. Their fields should be tracked by the owning StructType.

Comment 1

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Root StructType FieldInfo properties.

First attempt. Is this the right way?

Comment 2

[Terrence Cole [:terrence]]

Comment on attachment 781968 [details] [diff] [review]
Root StructType FieldInfo properties.

Review of attachment 781968 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with nits applied and followup bugs filed.

::: js/src/builtin/BinaryData.cpp
@@ +187,4 @@
>      size_t offset;
> +
> +    inline FieldInfo()
> +        : type(NULL), offset(0)

Methods with their body defined in the class are implicitly inline, so you can remove |inline| here. Also, the default constructor for type should work here. I would write this as:

FieldInfo() : offset(0) {}

@@ +191,5 @@
> +    {
> +    }
> +
> +    inline FieldInfo(const FieldInfo &o)
> +        : name(o.name.get()), type(o.type), offset(o.offset)

We should just make HeapId copyable. I found another bug while investigating, so I'm going to fix this in bug 885954. Once that is done, we can remove this whole method. Go ahead and land this as in and I'll fix it when I fix HeapId.

You can drop inline here as well.

@@ +272,5 @@
>      for (uint32_t i = 0; i < fieldList1->size(); ++i) {
>          FieldInfo fieldInfo1 = fieldList1->at(i);
>          FieldInfo fieldInfo2 = fieldList2->at(i);
>  
> +        if (fieldInfo1.name.get() != fieldInfo2.name.get())

We should add operator!= to HeapId. I'll do that out-of-line.

@@ +1706,5 @@
>  
> +void
> +StructType::trace(JSTracer *tracer, JSObject *obj)
> +{
> +    FieldList *fieldList = static_cast<FieldList *>(obj->getPrivate());

In a separate bug: please create a static GetFieldList function at the top of this file, move the getPrivate and static cast there, and assert at the top of the new method that the object is of the correct type. Then replace every other instance of this access in the file. with the new function.

@@ +1710,5 @@
> +    FieldList *fieldList = static_cast<FieldList *>(obj->getPrivate());
> +    JS_ASSERT(fieldList);
> +    for (FieldList::iterator it = fieldList->begin(); it != fieldList->end(); ++it) {
> +        gc::MarkId(tracer, &(it->name), "structtype.field.name");
> +        MarkObject(tracer, &(it->type), "structtype.field.type");

O_o These are in the same namespace. If you were able to compile with bare MarkObject, you should be fine with removing the gc:: from MarkId.

Comment 3

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/318216ea648d

Comment 4

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/318216ea648d