Closed Bug 1592712 Opened 2 years ago Closed 3 months ago

Rename JSOp::IfNe and JSop::IfEq

Categories

(Core :: JavaScript Engine, task, P3)

task

Tracking

()

RESOLVED FIXED
87 Branch
Tracking Status
firefox87 --- fixed

People

(Reporter: jorendorff, Assigned: jandem)

Details

Attachments

(2 files)

JSOP_IFEQ means "jump if false".

JSOP_IFNE means "jump if true".

These names are old. The original idea was probably an analogy to assembly language; x86 has je and jne. But in SpiderMonkey this is not a good analogy. These opcode names are super confusing to newcomers. I've wanted to change them forever.

Bikeshed zone: So what should the new names be? I think JSOP_JUMP_IF_TRUE/FALSE. A bit long, but clear. I like JSOP_JT, but then we also have JSOP_JUMPTARGET. JUMP_IF_TRUTHY would be more technically correct, but it would make everyone go "wtf" the first time they see it, and then it'd be ugly forever.

To illustrate why I think this is confusing: the statement if (x != 0) { f(x); } has bytecode that says getlocal x; zero; ne; ifeq +24

That ne; ifeq sequence is pretty bad imho.

+10. Whenever I see JSOP_IFEQ or JSOP_IFNE I pause and think "which one is this again?".

JSOP_JUMP_IF_TRUE seems reasonable. At some point I want to rename the bytecode ops so we have JSOp::JumpIfTrue and that's really not that long.

FWIW:

  • JSC uses jtrue and jfalse.
  • V8 uses JumpIfTrue and JumpIfFalse.
  • And Java uses ifeq and ifne.
Priority: -- → P3

IfEq and IfNe are very confusing.

Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Summary: Rename JSOP_IFNE and JSOP_IFEQ → Rename JSOp::IfNe and JSop::IfEq
Pushed by jdemooij@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a670453eafa3
part 1 - Rename JSOp::IfEq to JSOp::JumpIfFalse. r=arai
https://hg.mozilla.org/integration/autoland/rev/af48b695bea3
part 2 - Rename JSOp::IfNe to JSOp::JumpIfTrue. r=arai
Status: ASSIGNED → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → 87 Branch
You need to log in before you can comment on or make changes to this bug.