1459845 - Make IfThenElseEmitter methods clearer and fix comment

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P3)

Description

[Tooru Fujisawa [:arai]]

Followup for bug 1456039.

the comment added in bug 1456039 was wrong that, it says 
IfThenElseEmitter cannot be used for cascading ifelse's, but actually it can.

Comment 1

[Tooru Fujisawa [:arai]]

and I guess, it's better renaming/adding methods to clarify the structure, even if the method doesn't emit any bytecode.
(like, something to call before emitting condition)

Comment 2

[Tooru Fujisawa [:arai]]

I checked again the current usage, and apparently it's not a good idea to call some method before emitting condition,
because the condition can already be on the stack, as the result of previous operation.

I'll add emitElseIf method to clarify the else-if chain, and also rename emitIf* to emitThen* for clarity.

Comment 3

[Tooru Fujisawa [:arai]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a5d5c5dbc2c1266ab74f1990738a36dbcaf767b7

Comment 4

[Tooru Fujisawa [:arai]]

some more background here is,
while experimenting with bug 1455548, I noticed that it's better naming helper class methods like following:

  if (!helper.emitSomething())
    return false;

  if (bce.emit(parse_node_of_something))
    return false;

at least for control structure, so that it's clear that these methods should be called before emitting corresponding node.
and current IfThenElseEmitter doesn't follow that style.

Comment 5

[Tooru Fujisawa [:arai]]

Attachment: Part 1: Fix the comment for IfThenElseEmitter with cascading if-else.

Just fixed the comment to reflect the actual usage and the state transition.

Comment 6

[Tooru Fujisawa [:arai]]

Attachment: Part 2: Make IfThenElseEmitter::State an enum class.

Comment 7

[Tooru Fujisawa [:arai]]

Attachment: Part 3: Add MOZ_MUST_USE to IfThenElseEmitter methods.

Comment 8

[Tooru Fujisawa [:arai]]

Attachment: Part 4: Add IfThenElseEmitter::emitElseIf to clarify cascading else-if blocks.

If emitting cascading else-if blocks, the code emitted after emitElse is the condition for the next branch,
so added emitElseIf, to clarify it's different than non-cascading else.
(actually there's no difference between emitElseIf and emitElse, in term of the emitted code)

Comment 9

[Tooru Fujisawa [:arai]]

Attachment: Part 5: Rename IfThenElseEmitter::{emitIf,emitIfElse} to IfThenElseEmitter::{emitThen,emitThenElse}.

then, to align with WIP patches for other helper classes [1] for control structure,
renamed emitIf to emitThen (because the code emitted after that call is then-clause),
and emitIfElse to emitThenElse (actually I'm not sure if this name is good, but it's better having separate methods for then-without-else and then-with-else, to avoid modifying source note later)

[1] https://hg.mozilla.org/try/pushloghtml?changeset=2259f80dff35ee8f5e703602924906c67bc0e54d

Comment 10

[Tooru Fujisawa [:arai]]

Attachment: Part 6: Make IfThenElseEmitter::State debug only.

Then, just noticed state_ field is not necessary for non-debug code.

Comment 11

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

Comment on attachment 8974222 [details] [diff] [review]
Part 2: Make IfThenElseEmitter::State an enum class.

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

::: js/src/frontend/BytecodeEmitter.cpp
@@ +2063,5 @@
>          // Emit an annotated branch-if-false around the then part.
> +        SrcNoteType type = nextState == State::If
> +                           ? SRC_IF
> +                           : nextState == State::IfElse
> +                           ? SRC_IF_ELSE

You know our style conventions better than I do, but shouldn't the second ?: be indented?

Comment 12

[Tooru Fujisawa [:arai]]

Thank you for reviewing :)

(In reply to David Teller [:Yoric] (please use "needinfo") from comment #11)
> ::: js/src/frontend/BytecodeEmitter.cpp
> @@ +2063,5 @@
> >          // Emit an annotated branch-if-false around the then part.
> > +        SrcNoteType type = nextState == State::If
> > +                           ? SRC_IF
> > +                           : nextState == State::IfElse
> > +                           ? SRC_IF_ELSE
> 
> You know our style conventions better than I do, but shouldn't the second ?:
> be indented?

I haven't seen extra indentation around such situation.
but it might be better using switch or something, to more clarify the relation between condition and result value.

Comment 13

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

Comment on attachment 8974286 [details] [diff] [review]
Part 6: Make IfThenElseEmitter::State debug only.

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

::: js/src/frontend/BytecodeEmitter.cpp
@@ +2191,3 @@
>              if (!bce_->emitJumpTargetAndPatch(jumpAroundThen_))
>                  return false;
>          }

Could you have a slightly stronger `MOZ_ASSERT((state_ == State::Then) == (jumpAroundThen_.offset != -1))` and a one-line comment explaining the meaning of `jumpAroundThen_.offset != -1`?

Comment 14

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/51e2c8aea2e19f263fb43cb91f8b810048ee9f37
Bug 1459845 - Part 1: Fix the comment for IfThenElseEmitter with cascading if-else. r=Yoric

https://hg.mozilla.org/integration/mozilla-inbound/rev/15f3eea9a5bb926e9d8ba33ead448ae628130203
Bug 1459845 - Part 2: Make IfThenElseEmitter::State an enum class. r=Yoric

https://hg.mozilla.org/integration/mozilla-inbound/rev/1bf4b8c47cbef84ebac166046f5e61ec71cb7962
Bug 1459845 - Part 3: Add MOZ_MUST_USE to IfThenElseEmitter methods. r=Yoric

https://hg.mozilla.org/integration/mozilla-inbound/rev/31606aa882a925c60bcbef04d09910ba979d1ac4
Bug 1459845 - Part 4: Add IfThenElseEmitter::emitElseIf to clarify cascading else-if blocks. r=Yoric

https://hg.mozilla.org/integration/mozilla-inbound/rev/901ab55af4f3c894f658a79ef25f1bf54654ee18
Bug 1459845 - Part 5: Rename IfThenElseEmitter::{emitIf,emitIfElse} to IfThenElseEmitter::{emitThen,emitThenElse}. r=Yoric

https://hg.mozilla.org/integration/mozilla-inbound/rev/0710b2d9b60efc8459449111bc2b511a83da50df
Bug 1459845 - Part 6: Make IfThenElseEmitter::State debug only. r=Yoric

Comment 15

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/632399edfe3afd9d6956754cf5cc6296b0968724
Bug 1459845 - followup: Properly enclose debug-only variable. r=bustage CLOSED TREE

Comment 16

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/51e2c8aea2e1
https://hg.mozilla.org/mozilla-central/rev/15f3eea9a5bb
https://hg.mozilla.org/mozilla-central/rev/1bf4b8c47cbe
https://hg.mozilla.org/mozilla-central/rev/31606aa882a9
https://hg.mozilla.org/mozilla-central/rev/901ab55af4f3
https://hg.mozilla.org/mozilla-central/rev/0710b2d9b60e
https://hg.mozilla.org/mozilla-central/rev/632399edfe3a