Closed
Bug 573792
Opened 14 years ago
Closed 14 years ago
Narcissus JS: need fallback for when __applyConstructor__ is not available.
Categories
(Other Applications Graveyard :: Narcissus, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: taustin, Assigned: taustin)
Details
(Whiteboard: fixed-in-tracemonkey)
Attachments
(1 file, 2 obsolete files)
1.71 KB,
patch
|
gal
:
review+
|
Details | Diff | Splinter Review |
Since __applyConstructor__ is only available in special builds, we need a fallback solution. The attached patch will work for instances of up to 9 arguments to a constructor, which will pass the bulk of the jstests.py unit tests: $ python jstests.py -d -j 4 ../narcShell.py -m narcissus.list -x narcFailuresFull.txt [1156| 6| 150] 100% ===============================================>| 439.3s FIXES narcissus/../js1_8_5/regress/regress-555246-1.js REGRESSIONS narcissus/../ecma/Array/15.4.2.1-2.js narcissus/../ecma/Array/15.4.2.1-3.js narcissus/../ecma/Array/15.4.4.4-1.js narcissus/../ecma/Array/15.4.5.1-1.js narcissus/../ecma/Array/15.4.5.2-2.js narcissus/../ecma_3/extensions/regress-103087.js FAIL With __applyConstructor__, no new tests fail: $ python jstests.py -d -j 4 ../narcShell.py -m narcissus.list -x narcFailuresFull.txt [1162| 0| 150] 100% ===============================================>| 446.1s FIXES narcissus/../js1_8_5/regress/regress-555246-1.js PASS
Comment 1•14 years ago
|
||
Can we eval our way out of this if we have more than N args? Its slow, but it would be a fallback (instead of the PANIC thing).
Assignee | ||
Comment 2•14 years ago
|
||
New patch using eval as a fallback for when __applyConstructor__ is not available. Performance actually seems pretty comparable.
Attachment #453125 -
Attachment is obsolete: true
Assignee | ||
Updated•14 years ago
|
Attachment #453226 -
Flags: review?(gal)
Comment 3•14 years ago
|
||
Comment on attachment 453226 [details] [diff] [review] Patch using eval in place of __applyConstructor__ >diff -r f0fa8aa7f049 js/narcissus/jsexec.js >--- a/js/narcissus/jsexec.js Tue Jun 22 15:37:18 2010 -0700 >+++ b/js/narcissus/jsexec.js Tue Jun 22 16:09:56 2010 -0700 >@@ -906,17 +906,27 @@ if (!('__call__' in Fp)) { > > REp.__defineProperty__('__call__', function (t, a, x) { > a = Array.prototype.splice.call(a, 0, a.length); > return this.exec.apply(this, a); > }, true, true, true); > > Fp.__defineProperty__('__construct__', function (a, x) { > a = Array.prototype.splice.call(a, 0, a.length); >- return this.__applyConstructor__(a); >+ if (this.__applyConstructor__){ >+ return this.__applyConstructor__(a); >+ } Newline. I think I would prefer a switch for 0..3 or so and then the eval hack and no mentioning of __applyConstructor__ at all. >+ // Fallback for when __applyConstructor__ is not available. >+ else { >+ var argStr = ""; >+ for (var i in a) { >+ argStr += 'a[' + i + '],'; >+ } >+ return eval('new this(' + argStr.slice(0,-1) + ');'); >+ } > }, true, true, true); > > // Since we use native functions such as Date along with host ones such > // as global.eval, we want both to be considered instances of the native > // Function constructor. > Fp.__defineProperty__('__hasInstance__', function (v) { > return v instanceof Function || v instanceof global.Function; > }, true, true, true);
Comment 4•14 years ago
|
||
r+ with comments means you have to fix the comments before pushing, but its close enough to the final result that you don't have to ask for review again.
Assignee | ||
Comment 5•14 years ago
|
||
Assignee: general → taustin
Attachment #453226 -
Attachment is obsolete: true
Attachment #457454 -
Flags: review?(gal)
Attachment #453226 -
Flags: review?(gal)
Comment 6•14 years ago
|
||
Comment on attachment 457454 [details] [diff] [review] Updated patch to eliminate appluyConstructor completely Maybe loop from 0..a.length-1? That seems clearer.
Attachment #457454 -
Flags: review?(gal) → review+
Assignee | ||
Comment 7•14 years ago
|
||
Changelist: http://hg.mozilla.org/tracemonkey/rev/11aefd0d4f37
Whiteboard: fixed-in-tracemonkey
Comment 8•14 years ago
|
||
(In reply to comment #6) > Comment on attachment 457454 [details] [diff] [review] > Updated patch to eliminate appluyConstructor completely > > Maybe loop from 0..a.length-1? That seems clearer. That is a good suggestion, since slice is costly even with dependent (prefix) string Also, please put a space on both sides of binary operators including = and <. /be
Updated•14 years ago
|
Component: JavaScript Engine → Narcissus
Keywords: narcissus
Product: Core → Other Applications
QA Contact: general → narcissus
Comment 9•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/11aefd0d4f37
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
•