1592712 - Rename JSOp::IfNe and JSop::IfEq

Status: RESOLVED FIXED

(Core :: JavaScript Engine, task, P3)

Description

[Jason Orendorff [:jorendorff]]

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.

Comment 1

[Jason Orendorff [:jorendorff]]

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.

Comment 2

[Jan de Mooij [:jandem]]

+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.

Comment 3

[André Bargull [:anba]]

FWIW:

• JSC uses jtrue and jfalse.
• V8 uses JumpIfTrue and JumpIfFalse.
• And Java uses ifeq and ifne.

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Bug 1592712 part 1 - Rename JSOp::IfEq to JSOp::JumpIfFalse. r?arai!

IfEq and IfNe are very confusing.

Comment 5

[Jan de Mooij [:jandem]]

Attachment: Bug 1592712 part 2 - Rename JSOp::IfNe to JSOp::JumpIfTrue. r?arai!

Depends on D104210

Comment 6

[Pulsebot]

Pushed by [e-mail]:
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

Comment 7

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/a670453eafa3
https://hg.mozilla.org/mozilla-central/rev/af48b695bea3