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
Created attachment 824605 [details] [diff] [review] Patch
Assignee: nobody → hv1989
Attachment #824605 - Flags: review?(jorendorff)
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...
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+
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
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
You need to log in before you can comment on or make changes to this bug.