Closed
Bug 324592
Opened 18 years ago
Closed 18 years ago
Add JS_LeaveLocalRootScopeWithResult API
Categories
(Core :: JavaScript Engine, defect, P1)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla1.9alpha1
People
(Reporter: brendan, Assigned: brendan)
Details
(Keywords: fixed1.8.1)
Attachments
(1 file, 1 obsolete file)
27.48 KB,
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
and internal equivalent, so we can pop a local root scope without leaving a return value or similar such "result" unrooted. /be
Assignee | ||
Updated•18 years ago
|
Status: NEW → ASSIGNED
Priority: -- → P1
Assignee | ||
Comment 1•18 years ago
|
||
Igor, please feel free to review too. The js_LeaveLocalRootScopeWithResult code needs to be read in full, so I used more unified diff context. /be
Attachment #209539 -
Flags: superreview?(shaver)
Attachment #209539 -
Flags: review?(mrbkap)
Comment 2•18 years ago
|
||
Comment on attachment 209539 [details] [diff] [review] proposed fix Throughout this patch, you use JSVAL_NULL as a "not a real return" value. I think that it's better to use JSVAL_VOID, since that isn't a gcthing, and therefore, won't be pushed onto the stack. >Index: jsxml.c >@@ -1466,33 +1466,33 @@ ParseNodeToXML(JSContext *cx, JSParseNod > if (js_PushLocalRoot(cx, cx->localRootStack, (jsval)xml) < 0) > goto fail; This is no longer necessary, right?
Assignee | ||
Comment 3•18 years ago
|
||
Thanks, how's this? /be
Attachment #209539 -
Attachment is obsolete: true
Attachment #209670 -
Flags: superreview?(shaver)
Attachment #209670 -
Flags: review?(mrbkap)
Attachment #209539 -
Flags: superreview?(shaver)
Attachment #209539 -
Flags: review?(mrbkap)
Comment 4•18 years ago
|
||
Comment on attachment 209670 [details] [diff] [review] proposed fix, v2 I'll never understand your reluctance to use JSVAL_VOID, but it doesn't matter, so r=mrbkap.
Attachment #209670 -
Flags: review?(mrbkap) → review+
Assignee | ||
Comment 5•18 years ago
|
||
(In reply to comment #4) > (From update of attachment 209670 [details] [diff] [review] [edit]) > I'll never understand your reluctance to use JSVAL_VOID, but it doesn't matter, > so r=mrbkap. Simple: JSVAL_VOID takes some work to synthesize (via immediate operands) or load (from a 32-bit constant). JSVAL_NULL is 0. So the test in the callee is cheaper than all the uses in the callers. /be
Assignee | ||
Comment 6•18 years ago
|
||
Checked into trunk. /be
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Updated•18 years ago
|
Flags: testcase-
Attachment #209670 -
Flags: superreview?(shaver)
You need to log in
before you can comment on or make changes to this bug.
Description
•