1456039 - Add comment to IfThenElseEmitter and TryEmitter

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P2)

Description

[Tooru Fujisawa [:arai]]

Before bug 1455548, we need detailed comment for both IfThenElseEmitter and TryEmitter, how to use them, especially without ParseNode.

Comment 1

[Tooru Fujisawa [:arai]]

Attachment: Part 1: Add comment for IfThenElseEmitter.

Are those comment sufficient for bug 1455548 purpose?

Comment 2

[Tooru Fujisawa [:arai]]

Attachment: Part 2: Add comment for TryEmitter.

Comment 3

[David Teller [:Yoric] - still alive but not very active]

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.

Comment 4

[David Teller [:Yoric] - still alive but not very active]

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.

Comment 5

[Tooru Fujisawa [:arai]]

Thank you for reviewing :)
I'll land them after bug 1451826

Comment 6

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d5df9c66d35f47256fc0a13ea77650238f44637b
Bug 1456039 - Part 1: Add comment for IfThenElseEmitter. r=Yoric

https://hg.mozilla.org/integration/mozilla-inbound/rev/ff765f9cd347ba2eabde3a54d74d35ffec495fc6
Bug 1456039 - Part 2: Add comment for TryEmitter. r=Yoric

Comment 7

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/d5df9c66d35f
https://hg.mozilla.org/mozilla-central/rev/ff765f9cd347