FunctionBox should inherit from ObjectBox, not forward to it

RESOLVED FIXED in mozilla18

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: ejpbruel, Unassigned)

Tracking

unspecified
mozilla18
x86
Mac OS X
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

6 years ago
Right now we have this rather weird organisation where if FunctionBox contains an Objectbox, the ObjectBox contains the FunctionBox it is contained in.

For Harmony modules, we will likely also end up with ModuleBoxes. Conceptually, both ModuleBox and FunctionBox are just a specialisation of ObjectBox. It would be much nicer to inherit both from ObjectBox, and make ObjectBox a tagged union to avoid the need for virtual functions.

Note that ModuleBox and FunctionBox also inherit from SharedContext, but from the perspective of ObjectBox, we don't care about this. All we care about is that the traceable stuff in both is visible in ObjectBox's union.
(Reporter)

Comment 1

6 years ago
Created attachment 666361 [details] [diff] [review]
Patch to be reviewed

Here's a patch that cleans up ObjectBox. Better encapsulation, and the relationship between FunctionBox and ObjectBox is much clearer this way. Didn't need any unions or type tags; the type of the underlying Object is enough to determine the type of the box.
Attachment #666361 - Flags: review?(jorendorff)
Comment on attachment 666361 [details] [diff] [review]
Patch to be reviewed

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

I did it the current way because I was wary of having multiple inheritance, but there's no real reason not to have it in this case and it does make things clearer.  Thanks.
Attachment #666361 - Flags: review?(jorendorff) → review+
https://hg.mozilla.org/mozilla-central/rev/ef321673c843
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Depends on: 797469
You need to log in before you can comment on or make changes to this bug.