Add JS_LeaveLocalRootScopeWithResult API

RESOLVED FIXED in mozilla1.9alpha1

Status

()

Core
JavaScript Engine
P1
normal
RESOLVED FIXED
12 years ago
11 years ago

People

(Reporter: brendan, Assigned: brendan)

Tracking

({fixed1.8.1})

Trunk
mozilla1.9alpha1
fixed1.8.1
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

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

/be
(Assignee)

Updated

12 years ago
Status: NEW → ASSIGNED
Priority: -- → P1
(Assignee)

Comment 1

12 years ago
Created attachment 209539 [details] [diff] [review]
proposed fix

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?
(Assignee)

Comment 3

12 years ago
Created attachment 209670 [details] [diff] [review]
proposed fix, v2

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+
(Assignee)

Comment 5

12 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

12 years ago
Checked into trunk.

/be
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED

Updated

12 years ago
Flags: testcase-
Landed on the 1.8 branch.
Keywords: fixed1.8.1
Attachment #209670 - Flags: superreview?(shaver)
You need to log in before you can comment on or make changes to this bug.