Closed
Bug 554550
Opened 14 years ago
Closed 14 years ago
remove lingering defaultValue calls with hint JSTYPE_OBJECT or JSTYPE_FUNCTION
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
People
(Reporter: luke, Assigned: gal)
References
Details
(Whiteboard: fixed-in-tracemonkey)
Attachments
(1 file, 1 obsolete file)
11.86 KB,
patch
|
brendan
:
review+
|
Details | Diff | Splinter Review |
I was wandering around defaultValue-land today and came across a couple of places where defaultValue is called with hint = JSTYPE_OBJECT or JSTYPE_FUNCTION. Apparently, without liveconnect around our necks, this is no longer needed and they can/should be removed. Uses include (but may not limited to): js_ValueToObject, js_ValueToFunction, fun_toStringHelper, js_fun_call, js_fun_apply
Assignee | ||
Comment 2•14 years ago
|
||
This is in the way of my iterator work.
Assignee | ||
Comment 3•14 years ago
|
||
Assignee: general → gal
Comment 4•14 years ago
|
||
Comment on attachment 438390 [details] [diff] [review] patch > JS_FRIEND_DATA(JSClass) js_CallClass = { > "Call", > JSCLASS_HAS_PRIVATE | > JSCLASS_HAS_RESERVED_SLOTS(CALL_CLASS_FIXED_RESERVED_SLOTS) | > JSCLASS_NEW_RESOLVE | JSCLASS_IS_ANONYMOUS | JSCLASS_MARK_IS_TRACE, > JS_PropertyStub, JS_PropertyStub, > JS_PropertyStub, JS_PropertyStub, > call_enumerate, (JSResolveOp)call_resolve, >- call_convert, NULL, >+ JS_ConvertStub, NULL, Use NULL not JS_ConvertStub -- we should crash if anyone gets its hands on a Call object and tries to convert it to anything. > js_ValueToFunction(JSContext *cx, jsval *vp, uintN flags) > { > jsval v; > JSObject *obj; > > v = *vp; >+ if (!VALUE_IS_FUNCTION(cx, v)) { > js_ReportIsNotFunction(cx, vp, flags); > return NULL; > } >+ return GET_FUNCTION_PRIVATE(cx, JSVAL_TO_OBJECT(v)); Great -- just wondering whether proxies will change this again. >@@ -358,18 +358,18 @@ js_ValueToIterator(JSContext *cx, uintN > > JS_ASSERT(!(flags & ~(JSITER_ENUMERATE | JSITER_FOREACH | JSITER_KEYVALUE))); > > /* JSITER_KEYVALUE must always come with JSITER_FOREACH */ > JS_ASSERT(!(flags & JSITER_KEYVALUE) || (flags & JSITER_FOREACH)); > > AutoValueRooter tvr(cx); > >- /* XXX work around old valueOf call hidden beneath js_ValueToObject */ > if (!JSVAL_IS_PRIMITIVE(*vp)) { >+ /* Common case. */ > obj = JSVAL_TO_OBJECT(*vp); > } else { > /* > * Enumerating over null and undefined gives an empty enumerator. > * This is contrary to ECMA-262 9.9 ToObject, invoked from step 3 of > * the first production in 12.6.4 and step 4 of the second production, > * but it's "web JS" compatible. Add "ES5 fixed for-in to match this de-facto standard." or words to that effect. ECMA-262 Fifth Edition 12.6.4 step 3 (ignore the "experValue" typos). [js_DefaultValue:] > default: > if (!obj->getClass()->convert(cx, obj, hint, &v)) > return JS_FALSE; > if (!JSVAL_IS_PRIMITIVE(v)) { > JSType type = JS_TypeOfValue(cx, v); >- if (type == hint || >- (type == JSTYPE_FUNCTION && hint == JSTYPE_OBJECT)) { >- goto out; >- } >- if (!js_TryMethod(cx, obj, cx->runtime->atomState.toStringAtom, 0, >- NULL, &v)) { >+ if (type != hint && >+ !js_TryMethod(cx, obj, cx->runtime->atomState.toStringAtom, 0, NULL, &v)) { If type != hint but !JSVAL_IS_PRIMITIVE(v) and type = JS_TypeOfValue(cx, v), then type must be JSTYPE_OBJECT or JSTYPE_FUNCTION. But by the assertion at the top of js_DefaultValue that this patch adds, hint cannot be JSTYPE_OBJECT or JSTYPE_FUNCTION, therefore type != hint is always true. What's really going on here is that we want to fall back on js_TryMethod(...toString...) if convert (AKA tryValueOf) succeeded (didn't throw/report/return-false) but did not give a primitive result (of any primitive type). So maybe just assert that type != hint and try toString. /be
Assignee | ||
Comment 5•14 years ago
|
||
Attachment #438390 -
Attachment is obsolete: true
Assignee | ||
Updated•14 years ago
|
Attachment #438392 -
Flags: review?(brendan)
Comment 6•14 years ago
|
||
Comment on attachment 438392 [details] [diff] [review] patch r=me, looks great -- but what is up with that test failure you mentioned on IRC? /be
Attachment #438392 -
Flags: review?(brendan) → review+
Assignee | ||
Updated•14 years ago
|
Whiteboard: fixed-in-tracemonkey
Comment 7•14 years ago
|
||
Cite it! :-P http://hg.mozilla.org/tracemonkey/rev/3de0a7da3a8e /be
Comment 8•14 years ago
|
||
(In reply to comment #6) > (From update of attachment 438392 [details] [diff] [review]) > r=me, looks great -- but what is up with that test failure you mentioned on > IRC? Anyone? Bueller? /be
Comment 9•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/3de0a7da3a8e
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•