Closed
Bug 837176
Opened 11 years ago
Closed 11 years ago
Simplify code flow in CheckSideEffects
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla21
People
(Reporter: Ms2ger, Assigned: Ms2ger)
Details
Attachments
(1 file)
9.69 KB,
patch
|
jorendorff
:
review+
|
Details | Diff | Splinter Review |
I think this is clearer.
Attachment #709127 -
Flags: review?(jorendorff)
Comment 1•11 years ago
|
||
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•11 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•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/79ebe1a5d8f4
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in
before you can comment on or make changes to this bug.
Description
•