Closed Bug 898623 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: nsm, Assigned: nsm)

References

Details

Attachments

(1 file)

FieldInfo's are stored on the heap. Their fields should be tracked by the owning StructType.
First attempt. Is this the right way?
Attachment #781968 - Flags: review?(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.
Attachment #781968 - Flags: review?(terrence) → review+
https://hg.mozilla.org/mozilla-central/rev/318216ea648d
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.