PersistentRooted should not publicly derive from LinkedListElement

RESOLVED FIXED in mozilla29

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jonco, Assigned: jonco)

Tracking

unspecified
mozilla29
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

5 years ago
This allows clients to the API to put these into their own linked lists - something that can only have unfortunate consequences.
(Assignee)

Comment 1

5 years ago
Created attachment 8361793 [details] [diff] [review]
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.
Attachment #8361793 - Flags: review?(sphink)
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.
Attachment #8361793 - Flags: review?(sphink) → review+
Created attachment 8363171 [details] [diff] [review]
Templatize on marking function
(Assignee)

Comment 4

5 years ago
(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.
Whiteboard: [leave open]
(Assignee)

Comment 6

5 years ago
Created attachment 8363647 [details] [diff] [review]
tidy-perisitent-rooted

How about this?  It's a version of your patch with typedefs to make it easier to read.
Attachment #8363171 - Attachment is obsolete: true
Attachment #8363647 - Flags: review?(sphink)
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! :)
Attachment #8363647 - Flags: review?(sphink) → review+
https://hg.mozilla.org/mozilla-central/rev/00b51ce701ff
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
You need to log in before you can comment on or make changes to this bug.