1237449 - Split emitSpread out of emitForOf for clarity

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

The intermixing hurts clarity more than it helps non-duplication, IMO.

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Copy emitForOf into a new emitSpread method, basically no other changes

Comment 2

[Jeff Walden [:Waldo]]

Attachment: Initial emitSpread cleanups. NOT REVIEWED YET

Comment 3

[Jeff Walden [:Waldo]]

Attachment: More emitSpread cleanups

Comment 4

[Jeff Walden [:Waldo]]

Attachment: Finish cleaning up emitForOf's code now that it's not also implementing the spread operation

Comment 5

[Eric Faust [:efaust]]

Comment on attachment 8704856 [details] [diff] [review]
Copy emitForOf into a new emitSpread method, basically no other changes

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

OK...

Comment 6

[Eric Faust [:efaust]]

Comment on attachment 8704859 [details] [diff] [review]
Initial emitSpread cleanups.  NOT REVIEWED YET

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

::: js/src/frontend/BytecodeEmitter.cpp
@@ +5549,3 @@
>  
>  #ifdef DEBUG
> +    auto loopDepth = this->stackDepth;

why is auto here better? In case we change the type of this->stackDepth?

Comment 7

[Eric Faust [:efaust]]

Comment on attachment 8704860 [details] [diff] [review]
More emitSpread cleanups

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

::: js/src/frontend/BytecodeEmitter.cpp
@@ +5544,5 @@
>      // Increment manually to reflect this.
>      this->stackDepth++;
>  
> +    ptrdiff_t beq;
> +    {

This syntactic block is just for logical readability, "this is the loop body". Up to you whether to keep it, not sure it gains that much.

Comment 8

[Eric Faust [:efaust]]

Comment on attachment 8704861 [details] [diff] [review]
Finish cleaning up emitForOf's code now that it's not also implementing the spread operation

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

These do indeed make it cleaner.

::: js/src/frontend/BytecodeEmitter.cpp
@@ +5608,5 @@
> +        if (!emitForInOrOfVariables(loopDecl))
> +            return false;
> +    }
> +
> +    // Compile the expression to the right of 'of'.

"Compile" is a terrible word here, while we're changing this.

Comment 9

[Jeff Walden [:Waldo]]

(In reply to Eric Faust [:efaust] from comment #6)
> > +    auto loopDepth = this->stackDepth;
> 
> why is auto here better? In case we change the type of this->stackDepth?

Not so much in case of a change, but because then neither reader/writer has to know the type of stackDepth -- they can just take as obvious that the variable has the right type, so long as it's only used in comparisons with values of like meaning.  That makes it easier to write such assertions, easier to read them, etc.

(Also, I did this partly for the reason you indirectly suggest.  We are inconsistent in several places wrt what type we use for stack depth: uint32_t in CGTryNoteList, int32_t *and* uint32_t in BytecodeEmitter, and for all I know other types elsewhere.  This probably should all be made consistent, but for comparison purposes it's all so much mental noise.)

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/af947b44dc7d
https://hg.mozilla.org/integration/mozilla-inbound/rev/0b4bc484328e
https://hg.mozilla.org/integration/mozilla-inbound/rev/368b826c8290
https://hg.mozilla.org/integration/mozilla-inbound/rev/6cc42896216e

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/af947b44dc7d
https://hg.mozilla.org/mozilla-central/rev/0b4bc484328e
https://hg.mozilla.org/mozilla-central/rev/368b826c8290
https://hg.mozilla.org/mozilla-central/rev/6cc42896216e