Closed Bug 324592 Opened 15 years ago Closed 15 years ago

Add JS_LeaveLocalRootScopeWithResult API

Categories

(Core :: JavaScript Engine, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla1.9alpha1

People

(Reporter: brendan, Assigned: brendan)

Details

(Keywords: fixed1.8.1)

Attachments

(1 file, 1 obsolete file)

and internal equivalent, so we can pop a local root scope without leaving a return value or similar such "result" unrooted.

/be
Status: NEW → ASSIGNED
Priority: -- → P1
Attached patch proposed fix (obsolete) — Splinter Review
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 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?
Attached patch proposed fix, v2Splinter Review
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 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+
(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
Checked into trunk.

/be
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Flags: testcase-
Landed on the 1.8 branch.
Keywords: fixed1.8.1
You need to log in before you can comment on or make changes to this bug.