Make IfThenElseEmitter methods clearer and fix comment

RESOLVED FIXED in Firefox 62

Status

()

defect
P3
trivial
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: arai, Assigned: arai)

Tracking

(Blocks 1 bug)

Trunk
mozilla62
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox62 fixed)

Details

Attachments

(6 attachments)

Assignee

Description

a year ago
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.
Assignee

Comment 1

a year ago
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)
Severity: normal → trivial
Summary: Fix the comment for IfThenElseEmitter → Make IfThenElseEmitter methods clearer and fix comment
Assignee

Comment 2

a year ago
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.
Assignee

Comment 4

a year ago
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.
Priority: -- → P3
Assignee

Comment 5

a year ago
Just fixed the comment to reflect the actual usage and the state transition.
Attachment #8974221 - Flags: review?(dteller)
Assignee

Comment 8

a year ago
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)
Attachment #8974224 - Flags: review?(dteller)
Assignee

Comment 9

a year ago
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
Attachment #8974225 - Flags: review?(dteller)
Assignee

Updated

a year ago
See Also: → 1460126
Assignee

Updated

a year ago
Blocks: 1455548
Assignee

Comment 10

a year ago
Then, just noticed state_ field is not necessary for non-debug code.
Attachment #8974286 - Flags: review?(dteller)
Attachment #8974221 - Flags: review?(dteller) → review+
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?
Attachment #8974222 - Flags: review?(dteller) → review+
Assignee

Comment 12

a year ago
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.
Attachment #8974223 - Flags: review?(dteller) → review+
Attachment #8974225 - Flags: review?(dteller) → review+
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`?
Attachment #8974286 - Flags: review?(dteller) → review+
Attachment #8974224 - Flags: review?(dteller) → review+
Assignee

Comment 14

a year ago
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
Assignee

Comment 15

a year ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/632399edfe3afd9d6956754cf5cc6296b0968724
Bug 1459845 - followup: Properly enclose debug-only variable. r=bustage CLOSED TREE
You need to log in before you can comment on or make changes to this bug.