Closed
Bug 584787
Opened 14 years ago
Closed 14 years ago
Node shouldn't extend Array
Categories
(Other Applications Graveyard :: Narcissus, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: dherman, Assigned: dherman)
References
Details
(Whiteboard: fixed-in-tracemonkey)
Attachments
(2 files, 1 obsolete file)
20.12 KB,
patch
|
Details | Diff | Splinter Review | |
54.38 KB,
patch
|
pcwalton
:
review+
|
Details | Diff | Splinter Review |
This is an abuse of inheritance. Some nodes have array properties, but not all nodes are themselves arrays. Dave
Assignee | ||
Comment 1•14 years ago
|
||
Thanks to bug 599159, this bug blocks pretty much everything for Narcissus. Dave
Assignee: nobody → dherman
Assignee | ||
Comment 2•14 years ago
|
||
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 3•14 years ago
|
||
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.
Comment 4•14 years ago
|
||
>- 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
Assignee | ||
Comment 5•14 years ago
|
||
> >- 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
Comment 6•14 years ago
|
||
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.
Comment 7•14 years ago
|
||
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.
Assignee | ||
Comment 8•14 years ago
|
||
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
Assignee | ||
Comment 9•14 years ago
|
||
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)
Comment 10•14 years ago
|
||
> 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 11•14 years ago
|
||
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+
Assignee | ||
Comment 12•14 years ago
|
||
http://hg.mozilla.org/tracemonkey/rev/3cb0b8e90cf3
Whiteboard: fixed-in-tracemonkey
Comment 15•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/3cb0b8e90cf3
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Updated•5 years ago
|
Product: Other Applications → Other Applications Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•