Closed
Bug 331429
Opened 19 years ago
Closed 19 years ago
Implement an apply that can be composed with new
Categories
(Core :: JavaScript Engine, defect, P4)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla1.9alpha1
People
(Reporter: mrbkap, Assigned: mrbkap)
Details
Attachments
(2 files, 2 obsolete files)
1.80 KB,
patch
|
brendan
:
review+
|
Details | Diff | Splinter Review |
12.48 KB,
patch
|
brendan
:
review+
|
Details | Diff | Splinter Review |
In Narcissus, we currently have this absolutely awful switch statement in Function.prototype.__construct__ to get around the lack of something like apply/call for construction. It seems like it would be cleaner and easier to just implement the special semantics around |new| (e.g., creating a new object with the proper prototype, etc.) and use __call__ to finish it off (like global.Function.__construct__). If this won't work for some reason that I'm not seeing, then we should implement a native __construct__ type function that does what apply or call do for regular calls for construction.
Comment 1•19 years ago
|
||
__construct__ ain't __call__. See Date and Array, oh and one other I forget all the time. /be
Comment 2•19 years ago
|
||
I've copped to this before: what JS really wants is an apply operator that can be composed with new. So you could 'apply new C(obj, args)' and get the equivalent of 'new C(args[0], args[1], ...)' where obj is the newly created object of C's class, and args is an array or arguments object. /be
Assignee | ||
Comment 3•19 years ago
|
||
Right, those and RegExp. The core problem is (of course) that Narcissus can't control whether fp->flags has the JSFRAME_CONSTRUCTING bit set, that's the core of the problem. I don't seem to recall there being a proposal that covers this for ECMAScript v4, but for Narcissus' purposes, instead of adding a new keyword (apply), why don't we add Function.prototype.__construct__ (Narcissus only) that does the Right Thing.
Summary: Why don't we implement __construct__ in terms of __call__? → Implement an apply that can be composed with new
Assignee | ||
Comment 4•19 years ago
|
||
This implements Function.prototype.__applyConstruct__.
Attachment #218584 -
Flags: review?(brendan)
Assignee | ||
Updated•19 years ago
|
Assignee: general → mrbkap
Assignee | ||
Updated•19 years ago
|
Status: NEW → ASSIGNED
Priority: -- → P4
Target Milestone: --- → mozilla1.9alpha
Assignee | ||
Comment 5•19 years ago
|
||
...using __applyConstruct__...
Attachment #218585 -
Flags: review?(brendan)
Assignee | ||
Comment 6•19 years ago
|
||
These are mainly cosmetic changes.
Attachment #218584 -
Attachment is obsolete: true
Attachment #218732 -
Flags: review?(brendan)
Attachment #218584 -
Flags: review?(brendan)
Comment 7•19 years ago
|
||
Comment on attachment 218585 [details] [diff] [review] Narcissus changes With your nit-picky renaming :-P. /be
Attachment #218585 -
Flags: review?(brendan) → review+
Comment 8•19 years ago
|
||
Comment on attachment 218732 [details] [diff] [review] Second pass at SpiderMonkey changes > JSBool >+js_InvokeConstructor(JSContext *cx, uintN argc) Please define this right after js_StrictlyEqual's definition. >+{ >+ jsval *vp; >+ JSFunction *fun; >+ JSObject *obj, *obj2, *proto, *parent; >+ JSClass *clasp, *funclasp; >+ jsval lval, rval; Nit: swap last two lines to match first-def order. >+ vp = cx->fp->sp - (2 + argc); D'oh, how about making vp a param, so the common cases (JSOP_NEW/JSOP_NEWINIT) don't have to pay a gratuitous recomputation cost? Make the odd case (fun_applyConstructor) pay (once only in that path too). >+#ifdef NARCISSUS >+static JSBool >+fun_applyConstructor(JSContext *cx, JSObject *obj, uintN argc, jsval *argv, >+ jsval *rval) >+{ >+ JSObject *args; >+ uintN len, i; Minor whining about deviation from aobj/length in fun_apply :-P. /be
Attachment #218732 -
Flags: review?(brendan)
Assignee | ||
Comment 9•19 years ago
|
||
Attachment #218732 -
Attachment is obsolete: true
Attachment #218740 -
Flags: review?(brendan)
Comment 10•19 years ago
|
||
Comment on attachment 218740 [details] [diff] [review] Updated >+ /* Check the return value and update obj from it. */ >+ rval = *vp; >+ if (JSVAL_IS_PRIMITIVE(rval)) { >+ if (!fun && JS_VERSION_IS_ECMA(cx)) { >+ /* native [[Construct]] returning primitive is error */ >+ JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, >+ JSMSG_BAD_NEW_RESULT, >+ js_ValueToPrintableString(cx, rval)); >+ return JS_FALSE; >+ } >+ *vp = OBJECT_TO_JSVAL(obj); >+ } else { >+ obj = JSVAL_TO_OBJECT(rval); >+ } No need for else clause here (also change comment at top of this cited hunk to say "Check the return value and if primitive, force it to be obj" or some such). Fix that, and r=me -- thanks. /be
Attachment #218740 -
Flags: review?(brendan) → review+
Assignee | ||
Comment 11•19 years ago
|
||
Checked in.
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•