Remove JSOP_POPV and JSOP_STOP

RESOLVED FIXED in mozilla28

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: h4writer, Assigned: h4writer)

Tracking

unspecified
mozilla28
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
The following are equivalent (in what they do):
JSOP_POPV and JSOP_SETRVAL
JSOP_STOP and JSOP_RETRVAL

So it makes sense to merge them.

I choose to use JSOP_SETRVAL instead of JSOP_POPV, since the name JSOP_SETRVAL gives more information. 
I choose to use JSOP_STOP instead of JSOP_RETRVAL, since JSOP_STOP is also used when JSOP_SETRVAL isn't used. So it made more sense to me (it returns undefinedValue in that case).

Most changes are trivial (just remove the other opcode). Except for baseline. Here JSOP_STOP was used to know the end of the script. So I had to change that to test script->code + script->length instead
(Assignee)

Comment 1

5 years ago
Created attachment 824605 [details] [diff] [review]
Patch
Assignee: nobody → hv1989
Attachment #824605 - Flags: review?(jorendorff)
(Assignee)

Comment 2

5 years ago
Just had a discussion on IRC with jandem,

He would prefer JSOP_RETRVAL and drop JSOP_STOP.
Reason: it seems strange to see JSOP_STOP in the middle of a script.

For me it is the same. My initial thought was to use JSOP_STOP based on my explanation above. But I can change to JSOP_RETRVAL if others think it is better...
Attachment #824605 - Attachment is patch: true
Comment on attachment 824605 [details] [diff] [review]
Patch

Review of attachment 824605 [details] [diff] [review]:
-----------------------------------------------------------------

OK, looks good.

::: js/src/jit/BaselineCompiler.cpp
@@ +688,2 @@
>                  break;
>              pc += GetBytecodeLength(pc);

Maybe swap the order of the if-statement and the `pc +=` statement, to avoid calling GetBytecodeLength twice.

@@ +742,3 @@
>              break;
>  
>          pc += GetBytecodeLength(pc);

Same here.
Attachment #824605 - Flags: review?(jorendorff) → review+
(Assignee)

Comment 4

5 years ago
So in the end I removed jsop_stop (RIP) and we now use jsop_retrval instead. Since Jan and Jason both thought it made more sense.

https://hg.mozilla.org/integration/mozilla-inbound/rev/0cdec2743961
Summary: Remove JSOP_POPV and JSOP_RETRVAL → Remove JSOP_POPV and JSOP_STOP
https://hg.mozilla.org/mozilla-central/rev/0cdec2743961
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28

Updated

4 years ago
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.