1476203 - Handle column number in IfEmitter

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P3)

Description

[Tooru Fujisawa [:arai]]

https://searchfox.org/mozilla-central/rev/6f86cc3479f80ace97f62634e2c82a483d1ede40/js/src/frontend/BytecodeEmitter.cpp#4581-4584

this should be moved into new IfEmitter::emitTest method, in order to keep the same structure in multiple places which is added in future.

Comment 1

[Tooru Fujisawa [:arai]]

Attachment: Separate IfEmitter and CondEmitter.

In order to pass the position of `if`, we need to call some method before emitting the `cond`, for `if (cond) ...`.
(actually that matches to other emitters, which follows "emitSomething(); emit(something);" way)
Then, the position applies only to IfEmitter for if/if-else, while the current class shares the methods among InternalIfEmitter and conditional expression.

So, I splitted IfEmitter into IfEmitter and CondEmitter, with additional BranchEmitterBase which provides shared operations.
also added IfEmitter::emitIf for specifying the position, which is called before emitting the `cond` part, and also changed the meaning of emitCond() method which now is called at the same point as emitIf (which is no-op, but to make the interface similar), and now conditional expression has to call emitThenElse before emitting `then_expr`.
Then, InternalIfEmitter doesn't have to call emitIf, since there's no syntactic `if` which has position, and also the `cond` value can already be on the stack, which is the result of the previous operation.

now the usage is following:

   `if (c1) b1 else if (c2) b2 else b3`
     IfEmitter ifThenElse(this);
     ifThen.emitIf(Some(offset_of_if));
     emit(c1);
     ifThenElse.emitThenElse();
     emit(b1);
     ifThenElse.emitElseIf(Some(offset_of_if));
     emit(c2);
     ifThenElse.emitThenElse();
     emit(b2);
     ifThenElse.emitElse();
     emit(b3);
     ifThenElse.emitEnd();

   `if (cond) then_block else else_block`
     emit(cond);
     InternalIfEmitter ifThenElse(this);
     ifThenElse.emitThenElse();
     emit(then_block);
     ifThenElse.emitElse();
     emit(else_block);
     ifThenElse.emitEnd();

   `cond ? then_expr : else_expr`
     CondEmitter condElse(this);
     condElse.emitCond();
     emit(cond);
     condElse.emitThenElse();
     emit(then_expr);
     condElse.emitElse();
     emit(else_expr);
     condElse.emitEnd();

Comment 2

[Jeff Walden [:Waldo]]

Comment on attachment 8995066 [details] [diff] [review]
Separate IfEmitter and CondEmitter.

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

::: js/src/frontend/IfEmitter.h
@@ +210,5 @@
> +    MOZ_MUST_USE bool emitThen();
> +    MOZ_MUST_USE bool emitThenElse();
> +
> +    MOZ_MUST_USE bool emitElse();
> +    MOZ_MUST_USE bool emitElseIf(const mozilla::Maybe<uint32_t>& ifPos);

Flip the order of these, seeing as emitElseIf gets called before emitElse?

Comment 3

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/125176893a0a113be1865f1d3efc1e38425ee0e6
Bug 1476203 - Separate IfEmitter and CondEmitter. r=jwalden

Comment 4

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/125176893a0a