Closed Bug 1456039 Opened 4 years ago Closed 4 years ago

Add comment to IfThenElseEmitter and TryEmitter

Categories

(Core :: JavaScript Engine, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: arai, Assigned: arai)

References

Details

Attachments

(2 files)

Before bug 1455548, we need detailed comment for both IfThenElseEmitter and TryEmitter, how to use them, especially without ParseNode.
Are those comment sufficient for bug 1455548 purpose?
Attachment #8970437 - Flags: review?(dteller)
Attachment #8970438 - Flags: review?(dteller)
Comment on attachment 8970437 [details] [diff] [review]
Part 1: Add comment for IfThenElseEmitter.

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

\o/ documentation \o/

::: js/src/frontend/BytecodeEmitter.cpp
@@ +1878,3 @@
>      enum State {
>          Start,
>          If,

Nit: I'd prefer a text explanation, e.g. does `If` mean that we have called `emitIf` or that we can call it? Same thing for other labels.
Attachment #8970437 - Flags: review?(dteller) → review+
Comment on attachment 8970438 [details] [diff] [review]
Part 2: Add comment for TryEmitter.

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

::: js/src/frontend/BytecodeEmitter.cpp
@@ +1547,5 @@
> +    // Whether the catch and finally blocks handle the frame's return value.
> +    // If UseRetVal is specified, the bytecode to clear return value with
> +    // `undefined` is emitted before the catch block and the finally block,
> +    // and also the bytecode to save/restore the return value is emitted
> +    // before/after the finally block.

Nit: An example would be useful. How do we know in which case we stand? Who gets to decide?

@@ +1623,2 @@
>      enum State {
>          Start,

Same remark as for the other patch: I personally find it more readable as an explanation attached to specific enum variants than a picture.
Attachment #8970438 - Flags: review?(dteller) → review+
Thank you for reviewing :)
I'll land them after bug 1451826
Priority: -- → P2
https://hg.mozilla.org/mozilla-central/rev/d5df9c66d35f
https://hg.mozilla.org/mozilla-central/rev/ff765f9cd347
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Depends on: 1459845
You need to log in before you can comment on or make changes to this bug.