Closed Bug 1237449 Opened 4 years ago Closed 4 years ago

Split emitSpread out of emitForOf for clarity

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox46 --- fixed

People

(Reporter: Waldo, Assigned: Waldo)

References

Details

Attachments

(4 files)

The intermixing hurts clarity more than it helps non-duplication, IMO.
Attachment #8704859 - Flags: review?(efaustbmo)
Attachment #8704860 - Flags: review?(efaustbmo)
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...
Attachment #8704856 - Flags: review?(efaustbmo) → review+
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?
Attachment #8704859 - Flags: review?(efaustbmo) → review+
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.
Attachment #8704860 - Flags: review?(efaustbmo) → review+
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.
Attachment #8704861 - Flags: review?(efaustbmo) → review+
(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.)
You need to log in before you can comment on or make changes to this bug.