961077 - PersistentRooted should not publicly derive from LinkedListElement

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jon Coppeard (:jonco)]

This allows clients to the API to put these into their own linked lists - something that can only have unfortunate consequences.

Comment 1

[Jon Coppeard (:jonco)]

Attachment: persistent-rooted-inheritance

Here's a patch to make this use private inheritance.  We need to make LinkedList<T> and LinkedListElement<T> friend classes so they can cast the base pointer down to a PersistentRooted pointer again.  I removed the existing typedefs for these types because they're never used.

Comment 2

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8361793 [details] [diff] [review]
persistent-rooted-inheritance

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

The one thing that bothers me about this is the call through a 'marker' function pointer instead of templatizing on the function. Can the optimizer eliminate that? If not, something like the attached patch on top of your patch would eliminate a level of indirection.

Comment 3

[Steve Fink [:sfink] [:s:]]

Attachment: Templatize on marking function

Comment 4

[Jon Coppeard (:jonco)]

(In reply to Steve Fink [:sfink] from comment #2)
> The one thing that bothers me about this is the call through a 'marker'
> function pointer instead of templatizing on the function. Can the optimizer
> eliminate that? If not, something like the attached patch on top of your
> patch would eliminate a level of indirection.

I agree, and I'm not sure of the answer.  I'm going to land the original patch and look into this further.

Comment 5

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b0fcf310b266

Comment 6

[Jon Coppeard (:jonco)]

Attachment: tidy-perisitent-rooted

How about this?  It's a version of your patch with typedefs to make it easier to read.

Comment 7

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8363647 [details] [diff] [review]
tidy-perisitent-rooted

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

Oh! It's clever how you merged the two mark functions into a single signature.

Now you just need to eliminate markChainIfNotNull by partially specializing markChain on pointer types to do the null check... never mind, this is good! :)

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b0fcf310b266

Comment 9

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/00b51ce701ff

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/00b51ce701ff