Closed Bug 1465999 Opened 3 years ago Closed 3 years ago

Add ExpressionStatementEmitter

Categories

(Core :: JavaScript Engine, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox64 --- fixed

People

(Reporter: arai, Assigned: arai)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Just like others (bug 1456006, bug 1456404),
we'll need a class to emit ExpressionStatement, which handles source note and POP/SETRVAL.
Priority: -- → P3
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.
Attachment #9010845 - Flags: review?(jwalden+bmo)
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 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?
Attachment #9010845 - Flags: review?(jwalden+bmo) → review+
https://hg.mozilla.org/mozilla-central/rev/1be0907510b9
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
You need to log in before you can comment on or make changes to this bug.