1465999 - Add ExpressionStatementEmitter

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P3)

Description

[Tooru Fujisawa [:arai]]

Just like others (bug 1456006, bug 1456404),
we'll need a class to emit ExpressionStatement, which handles source note and POP/SETRVAL.

Comment 1

[Tooru Fujisawa [:arai]]

Attachment: Add ExpressionStatementEmitter.

A small abstraction for expression statement + ValueUsage (pop/setrval).

ValueUsage is moved to its own header given that it's used from ParseNode independent part.

Comment 2

[Tooru Fujisawa [:arai]]

Comment on attachment 9010845 [details] [diff] [review]
Add ExpressionStatementEmitter.

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

::: js/src/frontend/ExpressionStatementEmitter.cpp
@@ +19,5 @@
> +  : bce_(bce),
> +    valueUsage_(valueUsage)
> +#ifdef DEBUG
> +  , state_(State::Start)
> +#endif

forgot to move this to header.
fixed locally.

Comment 3

[Jeff Walden [:Waldo]]

Comment on attachment 9010845 [details] [diff] [review]
Add ExpressionStatementEmitter.

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

::: js/src/frontend/ExpressionStatementEmitter.cpp
@@ +47,5 @@
> +    MOZ_ASSERT(state_ == State::Expr);
> +    MOZ_ASSERT(bce_->stackDepth == depth_ + 1);
> +
> +    if (valueUsage_ == ValueUsage::WantValue) {
> +        if (!bce_->emit1(JSOP_SETRVAL)) {

This seems more readable as

  JSOp op = valueUsage_ == ValueUsage::WantValue ? JSOP_SETRVAL : JSOP_POP;
  if (!bce_->emit1(op)) {
    return false;
  }

across four lines rather than nine.

::: js/src/frontend/ExpressionStatementEmitter.h
@@ +28,5 @@
> +//     ese.emitExpr(Some(offset_of_expr));
> +//     emit(expr);
> +//     ese.emitEnd();
> +//
> +//   `expr;` in eval script

It feels like you probably should just show this once, with a variable for the ValueUsage value, whose meaning you describe in prose.

::: js/src/frontend/ValueUsage.h
@@ +10,5 @@
> +namespace js {
> +namespace frontend {
> +
> +// Used to control whether JSOP_CALL_IGNORES_RV is emitted for function calls.
> +enum class ValueUsage {

{ on its own line.

@@ +22,5 @@
> +    IgnoreValue
> +};
> +
> +} /* namespace frontend */
> +} /* namespace js */

//-style comments?

Comment 4

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1be0907510b948b7ba0693dc8410d1348e88e5c5
Bug 1465999 - Add ExpressionStatementEmitter. r=Waldo

Comment 5

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/1be0907510b9