Closed Bug 1188197 Opened 4 years ago Closed 4 years ago

Allow PersistentRooted to store DynamicTraceable

Categories

(Core :: JavaScript: GC, defect)

defect
Not set

Tracking

()

RESOLVED INVALID
Tracking Status
firefox42 --- affected

People

(Reporter: terrence, Assigned: terrence)

References

Details

Attachments

(1 file)

No description provided.
Attachment #8639631 - Flags: review?(jimb)
Comment on attachment 8639631 [details] [diff] [review]
support_dynamictraceable_in_persistentrooted-v0.diff

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

Please update the comment just above DynamicTraceable:

// If a class containing GC pointers has (or can gain) a vtable, then it can be
// trivially used with Rooted/Handle/MutableHandle by deriving from
// DynamicTraceable and overriding |void trace(JSTracer*)|.

::: js/public/RootingAPI.h
@@ +994,5 @@
>                           private mozilla::LinkedListElement<PersistentRooted<T>>
>  {
> +    static_assert(!mozilla::IsConvertible<T, StaticTraceable*>::value &&
> +                  !mozilla::IsConvertible<T, DynamicTraceable*>::value,
> +                  "Rooted takes pointer or Traceable types but not Traceable* type");

Thanks, that's an easy mistake to make.

::: js/src/gc/RootMarking.cpp
@@ +301,2 @@
>  
> +    template <TraceFunction<T> TraceFn = TraceNullableRoot>

Ugh. Now you have TraceFunction, TraceFunc, and TraceFn. And I don't see where TraceFunc is even used. I guess it's handy to expose as part of the class? Would it work as

  using TraceFunc = TraceFunction<T>;

?

How about s/TraceFn/TraceFuncT/? And

  template <TraceFunc TraceFnT = TraceNullableRoot>

maybe?

@@ +327,5 @@
>                                               "PersistentRooted<Value>");
> +
> +    PersistentRootedMarker<ConcreteTraceable>::markChain<MarkDynamicTraceable>(trc,
> +        reinterpret_cast<mozilla::LinkedList<JS::PersistentRooted<ConcreteTraceable>>&>(
> +            roots.heapRoots_[THING_ROOT_DYNAMIC_TRACEABLE]),

Yeah, now I'm thinking root.typedHeapRoots<ConcreteTraceable>(THING_ROOT_DYNAMIC_TRACEABLE) would be worth it. (Is there some way of shrinking that down? Doesn't ConcreteTraceable imply THING_ROOT_DYNAMIC_TRACEABLE?)

::: js/src/jsapi-tests/testGCExactRooting.cpp
@@ +276,5 @@
>      CHECK(CheckMyHashMap(cx, map));
>  
> +    // Unfortunately, the type of get() needs to be non-const ref, so
> +    // we need to explicitly Move the storage.
> +    JS::PersistentRooted<MyHashMap> heapMap(cx, mozilla::Move(map.get()));

I don't really understand the comment (aren't there already both const and non-const get()'s defined? What needs to be a non-const ref, again?) But it doesn't really matter
Attachment #8639631 - Flags: review?(jimb) → review+
See bug 1189809.
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Flags: needinfo?(terrence)
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.