837176 - Simplify code flow in CheckSideEffects

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch v1

I think this is clearer.

Comment 1

[Jason Orendorff [:jorendorff]]

Comment on attachment 709127 [details] [diff] [review]
Patch v1

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

I agree. Thanks.

::: js/src/frontend/BytecodeEmitter.cpp
@@ +1484,5 @@
>           * Object instance and binding a readonly, permanent property in it
>           * (the object and binding can be detected and hijacked or captured).
>           * This is a bug fix to ES3; it is fixed in ES3.1 drafts.
>           */
>          *answer = false;

*answer is already necessarily false here. (In fact this function must never toggle *answer from true to false; if it did, the code in case PN_TERNARY would be broken.)

So I am in favor of deleting this assignment.

@@ +1505,2 @@
>              /* Generator-expressions are harmless if the result is ignored. */
>              *answer = false;

Same here. And the one other place in this function where we set *answer = false.

@@ +1537,5 @@
>               * because the left operand may be a property with a setter that
>               * has side effects.
>               *
>               * The only exception is assignment of a useless value to a const
>               * declared in the function currently being compiled.

Wow. This is really too much. We should delete this whole `if (pn->isAssignment())` block.

Are you up for it? Or should I file a follow-up bug?

Comment 2

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Jason Orendorff [:jorendorff] from comment #1)
> Comment on attachment 709127 [details] [diff] [review]
> Patch v1
> 
> Review of attachment 709127 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I agree. Thanks.
> 
> ::: js/src/frontend/BytecodeEmitter.cpp
> @@ +1484,5 @@
> >           * Object instance and binding a readonly, permanent property in it
> >           * (the object and binding can be detected and hijacked or captured).
> >           * This is a bug fix to ES3; it is fixed in ES3.1 drafts.
> >           */
> >          *answer = false;
> 
> *answer is already necessarily false here. (In fact this function must never
> toggle *answer from true to false; if it did, the code in case PN_TERNARY
> would be broken.)
> 
> So I am in favor of deleting this assignment.
> 
> @@ +1505,2 @@
> >              /* Generator-expressions are harmless if the result is ignored. */
> >              *answer = false;
> 
> Same here. And the one other place in this function where we set *answer =
> false.

And assert it's already false, I assume?

> @@ +1537,5 @@
> >               * because the left operand may be a property with a setter that
> >               * has side effects.
> >               *
> >               * The only exception is assignment of a useless value to a const
> >               * declared in the function currently being compiled.
> 
> Wow. This is really too much. We should delete this whole `if
> (pn->isAssignment())` block.
> 
> Are you up for it? Or should I file a follow-up bug?

I'd rather not make such a change to code I hardly understand, if you don't mind :)

Comment 3

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/79ebe1a5d8f4