Simplify code flow in CheckSideEffects

RESOLVED FIXED in mozilla21

Status

()

RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Ms2ger, Assigned: Ms2ger)

Tracking

Trunk
mozilla21
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

6 years ago
Created attachment 709127 [details] [diff] [review]
Patch v1

I think this is clearer.
Attachment #709127 - Flags: review?(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?
Attachment #709127 - Flags: review?(jorendorff) → review+
(Assignee)

Comment 2

6 years ago
(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 :)
(Assignee)

Comment 3

6 years ago
https://hg.mozilla.org/mozilla-central/rev/79ebe1a5d8f4
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Depends on: 846984
No longer depends on: 846984
You need to log in before you can comment on or make changes to this bug.