ecma262 icon indicating copy to clipboard operation
ecma262 copied to clipboard

Editorial: refactor ForIn/OfBodyEvaluation

Open bakkot opened this issue 2 years ago • 2 comments

Split out from #2842. This is a straight refactoring, just reording some ifs. The only part which isn't totally obvious is that previously there was the following if-else chain:

1. If _destructuring_ is *false*, then
  1. If _lhsRef_ is an abrupt completion, then
    1. Let _status_ be _lhsRef_.
  1. Else if _lhsKind_ is ~lexicalBinding~, then
    1. Let _status_ be Completion(InitializeReferencedBinding(_lhsRef_, _nextValue_)).
  1. Else,
    1. Let _status_ be Completion(PutValue(_lhsRef_, _nextValue_)).

If you read the original carefully, the only place that could make _lhsRef_ be an abrupt completion is this earlier bit:

1. If _lhsKind_ is either ~assignment~ or ~varBinding~, then
  1. If _destructuring_ is *false*, then
    1. Let _lhsRef_ be Completion(Evaluation of _lhs_). (It may be evaluated repeatedly.)

So it's safe to move the Let _status_ be _lhsRef_ bit into the ~assignment~/~varBinding~ + _destructuring_ is *false* branch in the refactoring.

(The Let _status_ be Completion(PutValue(_lhsRef_, _nextValue_)). also gets moved into that branch, but that one's more obviously fine, since it's explicitly guarded on "not ~lexicalBinding~" i.e. "is ~assignment~/~varBinding~.)

bakkot avatar Aug 08 '22 18:08 bakkot

Looks correct to me (apart from "Asert").

I'm curious why you flipped the If _destructuring_ conditions from *false* to *true*.

jmdyck avatar Aug 14 '22 19:08 jmdyck

I'm curious why you flipped the If _destructuring_ conditions from *false* to *true*.

A weak preference for if/else branches to be ordered "true" then "false", in the absence of some reason to do it the other way. Not something I care about enough to go out of my way to change in general, but since I was touching this anyway I figured I might as well.

bakkot avatar Aug 14 '22 19:08 bakkot