Closed Bug 1189809 Opened 4 years ago Closed 4 years ago

Remove DynamicTraceable and unify on StaticTraceable

Categories

(Core :: JavaScript: GC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: terrence, Assigned: terrence)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

It turns out that DynamicTraceable would require another word in the Rooted to do property, so it doesn't end up having any technical advantage over StaticTraceable. Given that the boilerplate for StaticTraceable is minimal, I think we should just unify on StaticTraceable.
Attachment #8641727 - Flags: review?(jcoppeard)
Comment on attachment 8641727 [details] [diff] [review]
remove_dynamictraceable-v0.diff

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

Looks good!

::: js/public/TraceableHashTable.h
@@ +41,5 @@
>    public:
>      explicit TraceableHashMap(AllocPolicy a = AllocPolicy()) : Base(a)  {}
>  
> +    static void trace(TraceableHashMap* map, JSTracer* trc) { map->trace(trc); }
> +    void trace(JSTracer* trc) {

Does the non-static trace() need to be public any more?

::: js/public/TraceableVector.h
@@ +48,5 @@
>          return Base::operator=(mozilla::Forward<TraceableVector>(vec));
>      }
>  
> +    static void trace(TraceableVector* vec, JSTracer* trc) { vec->trace(trc); }
> +    void trace(JSTracer* trc) {

Ditto for this one.
Attachment #8641727 - Flags: review?(jcoppeard) → review+
I suppose we could, but I'd prefer to keep it public for when we use these containers outside of [Persistent]Rooted. In fact I think we're already using this to do fields->trace(trc) in CTypes.
Duplicate of this bug: 1189910
https://hg.mozilla.org/mozilla-central/rev/b05f6533036e
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.