Closed Bug 584787 Opened 14 years ago Closed 14 years ago

Node shouldn't extend Array

Categories

(Other Applications Graveyard :: Narcissus, defect)

x86
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: dherman, Assigned: dherman)

References

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(2 files, 1 obsolete file)

This is an abuse of inheritance. Some nodes have array properties, but not all nodes are themselves arrays.

Dave
Thanks to bug 599159, this bug blocks pretty much everything for Narcissus.

Dave
Assignee: nobody → dherman
Attached patch Node !<: Array (obsolete) — Splinter Review
First attempt at a refactoring to add a |children| property to Node instances, rather than have Node subclass Array. Also fixed a few broken windows along the way:

- repaired bustage in njs caused by Shu's patch for bug 573569
- comment police
- added nice stack traces to uncaught exceptions in the REPL

The next draft of this patch will need Shu's help updating jsssa.js.

Dave
Comment on attachment 481264 [details] [diff] [review]
Node !<: Array

>From: Dave Herman <dherman@mozilla.com>
>
>bug 584787, r=?: Node shouldn't subclass Array
>
>diff --git a/js/narcissus/jsexec.js b/js/narcissus/jsexec.js
>--- a/js/narcissus/jsexec.js
>+++ b/js/narcissus/jsexec.js
>@@ -308,28 +308,28 @@ Narcissus.interpreter = (function() {
>-        var a, f, i, j, r, s, t, u, v;
>+        var a, c, f, i, j, r, s, t, u, v;

Off topic, but I hate this. Can we fix this in a followup bug? :)

Patch looks great to me so far! Thanks for finding the time to make this fix.
>-        var a, f, i, j, r, s, t, u, v;
>+        var a, b, c, d, e, f, g, h, i, j, k, l, m, n, o, p, q, r, s, t, u, v, w, x, y, z;

FTFY
> >-        var a, f, i, j, r, s, t, u, v;
> >+        var a, c, f, i, j, r, s, t, u, v;

Mmmm, everything tastes better with Fortran.

> >-        var a, f, i, j, r, s, t, u, v;
> >+        var a, b, c, d, e, f, g, h, i, j, k, l, m, n, o, p, q, r, s, t, u, v, w, x, y, z;
> 
> FTFY

Bug 584784 is what you're looking for. Yes, I will do it when I get a chance.

> Patch looks great to me so far! Thanks for finding the time to make this fix.
Np. I kinda had to since we've been dead in the water since Arrays got busted.

Dave
Attached patch jsssa patchSplinter Review
Dave, your patch looks good to me.

This patch has two changes: removes the "not yet supported" placeholder SSABuilder, and the required jsssa changes with the children change.
I haven't re-run the unit tests to see if anything new broke (it would be weird if this change broke anything new), and will do so after Dave combines our two patches into a single, glorious beacon of hope.
Comment on attachment 481406 [details] [diff] [review]
jsssa patch

>-                        var lhs = n[0];
>+                        //var lhs = n[0];

Nit: don't leave commented-out lines in the source. I'll remove this line in the updated patch.

Dave
Combines my patch with Shu's. No regressions, and acceptable perf decrease (on my machine, the test suite takes ~30sec longer).

Dave
Attachment #481264 - Attachment is obsolete: true
Attachment #481522 - Flags: review?(pwalton)
> Nit: don't leave commented-out lines in the source. I'll remove this line in
> the updated patch.

Oops, meant to delete that!
Comment on attachment 481522 [details] [diff] [review]
by our powers combined...

>From: Dave Herman <dherman@mozilla.com>
>
>bug 584787, r=pcwalton: Node shouldn't subclass Array
>
>diff --git a/js/narcissus/jsexec.js b/js/narcissus/jsexec.js
>--- a/js/narcissus/jsexec.js
>+++ b/js/narcissus/jsexec.js
>@@ -308,28 +308,28 @@ Narcissus.interpreter = (function() {
>           case IDENTIFIER:
>+            if (typeof n.resolve !== "function")
>+                throw n
>+

;

r=me with that.
Attachment #481522 - Flags: review?(pwalton) → review+
http://hg.mozilla.org/tracemonkey/rev/3cb0b8e90cf3
Whiteboard: fixed-in-tracemonkey
http://hg.mozilla.org/mozilla-central/rev/3cb0b8e90cf3
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Product: Other Applications → Other Applications Graveyard
You need to log in before you can comment on or make changes to this bug.