Editorial: For clarity, manipulate _record_.[[Field]] rather than its alias#2947
Merged
Editorial: For clarity, manipulate _record_.[[Field]] rather than its alias#2947
Conversation
bakkot
reviewed
Jan 10, 2023
| 1. If _key_ is *-0*<sub>𝔽</sub>, set _key_ to *+0*<sub>𝔽</sub>. | ||
| 1. Let _p_ be the Record { [[Key]]: _key_, [[Value]]: _value_ }. | ||
| 1. Append _p_ to _entries_. | ||
| 1. Append _p_ to _M_.[[MapData]]. |
Member
There was a problem hiding this comment.
Might as well get rid of the _entries_ aliases entirely at this point (in this and subsequent algorithms); it seems weirder to refer to the same value in two different ways in the same algorithm.
michaelficarra
approved these changes
Jan 11, 2023
Member
michaelficarra
left a comment
There was a problem hiding this comment.
LGTM other than https://github.com/tc39/ecma262/pull/2947/files#r1065284972
syg
approved these changes
Jan 11, 2023
1c73e94 to
c954174
Compare
c954174 to
6aa7ba8
Compare
jmdyck
added a commit
to jmdyck/ecma262
that referenced
this pull request
Feb 16, 2023
PR tc39#2947 changed 6 occurrences of: `the element of _execution_.[[EventsRecords]] whose ...` to: `the Agent Events Record of _execution_.[[EventsRecords]] whose ...` But there were also 2 pre-existing occurrences of `the Agent Events Record in _execution_.[[EventsRecords]] whose ...` with "in" rather than "of", in {Enter,Leave}CriticalSection. To achieve consistency, this commit changes the latter (with "in") to match the former (with "of"). Roughly speaking, this conforms to PR tc39#2941.
ljharb
pushed a commit
to jmdyck/ecma262
that referenced
this pull request
Feb 17, 2023
PR tc39#2947 changed 6 occurrences of: `the element of _execution_.[[EventsRecords]] whose ...` to: `the Agent Events Record of _execution_.[[EventsRecords]] whose ...` But there were also 2 pre-existing occurrences of `the Agent Events Record in _execution_.[[EventsRecords]] whose ...` with "in" rather than "of", in {Enter,Leave}CriticalSection. To achieve consistency, this commit changes the latter (with "in") to match the former (with "of"). Roughly speaking, this conforms to PR tc39#2941.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Even though aliases are "reference-like" per Algorithm Conventions, it is not always clear in context that steps like "append <value> to listAlias" and "remove <value> from listAlias" affect the Record whose field is referenced by listAlias—and to be honest, the presence of many such aliases doesn't even contribute very much in my opinion. But this PR is still split into meaningful commits in case the editors disagree with a subset of its changes.
Another alternative option would be to keep the aliases but make their behavior more explicit, as is done like "Let S be a reference to the list of waiters in WL." in RemoveWaiters.