Skip to content

Editorial: Unify 'replace' step into single format#3767

Open
jungwngkim wants to merge 1 commit intotc39:mainfrom
jungwngkim:unify-replace-step
Open

Editorial: Unify 'replace' step into single format#3767
jungwngkim wants to merge 1 commit intotc39:mainfrom
jungwngkim:unify-replace-step

Conversation

@jungwngkim
Copy link
Copy Markdown
Contributor

This PR unifies 'replace' step into a consistent form.

Currently there are two forms of 'replace element of a List' step in spec.

# Form 1 (4 times)
# grep "1\. Replace the element of .*" spec.html
1. Replace the element of {{ container }} whose value is {{ oldElement }} with an element whose value is {{ newElement }}.

# Form 2 (1 time)
# grep "1\. Replace .* in .* with .*\." spec.html
1. Replace {{ oldElement }} in {{ container }} with {{ newElement }}.

Form 1 is merged into Form 2, as Form 2 expresses same meaning with brevity.

For your reference, the 'replace element of an Object' is left as a separate form. (appears 2 times in spec)

1. Replace the property named _P_ of object _O_ with ...

Comment thread spec.html
Comment on lines 44116 to +44117
1. For each element _e_ of _S_.[[SetData]], do
1. Replace the element of _S_.[[SetData]] whose value is _e_ with an element whose value is ~empty~.
1. Replace _e_ in _S_.[[SetData]] with ~empty~.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively,

Suggested change
1. For each element _e_ of _S_.[[SetData]], do
1. Replace the element of _S_.[[SetData]] whose value is _e_ with an element whose value is ~empty~.
1. Replace _e_ in _S_.[[SetData]] with ~empty~.
1. Set _S_.[[SetData]] to a new empty List.

or

Suggested change
1. For each element _e_ of _S_.[[SetData]], do
1. Replace the element of _S_.[[SetData]] whose value is _e_ with an element whose value is ~empty~.
1. Replace _e_ in _S_.[[SetData]] with ~empty~.
1. Remove all elements from _S_.[[SetData]].

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a question on 1. Set _S_.[[SetData]] to a new empty List., since the spec states as below. If it isn't a problem, I'll include the update on Map.prototype.clear as well.

The existing [[SetData]] List is preserved because there may be existing Set Iterator objects that are suspended midway through iterating over that List.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, right, I thought there was a reason we didn't do that, but couldn't remember. Then the latter suggestion is fine.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would strongly prefer the spec language to continue setting [[MapData]]/[[SetData]] elements to ~empty~ rather than removing them, because the latter is in invitation for potential future refactorings to introduce spec bugs by failing to account for multiple consumers of the same List (hence multiple notes about "there may be existing {Map,Set} Iterator objects that are suspended midway through iterating" and "The value EMPTY is used as a specification device to indicate that an entry has been deleted. Actual implementations may take other actions such as physically removing the entry").

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In fact, I find the index notation used by e.g. Set.prototype.forEach to be even more clear. Thoughts on adopting that throughout the Set/WeakSet algorithms?

Suggested change
1. For each element _e_ of _S_.[[SetData]], do
1. Replace the element of _S_.[[SetData]] whose value is _e_ with an element whose value is ~empty~.
1. Replace _e_ in _S_.[[SetData]] with ~empty~.
1. Let _numEntries_ be the number of elements in _S_.[[SetData]].
1. Let _index_ be 0.
1. Repeat, while _index_ < _numEntries_,
1. Set _S_.[[SetData]][_index_] to ~empty~.
1. Set _index_ to _index_ + 1.

Comment thread spec.html
1. Set _r_.[[Value]] to ~empty~.
1. For each WeakSet _set_ such that _set_.[[WeakSetData]] contains _value_, do
1. Replace the element of _set_.[[WeakSetData]] whose value is _value_ with an element whose value is ~empty~.
1. Replace _value_ in _set_.[[WeakSetData]] with ~empty~.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer we emphasis that we know there's only one such element.

Suggested change
1. Replace _value_ in _set_.[[WeakSetData]] with ~empty~.
1. Replace the sole occurrence of _value_ in _set_.[[WeakSetData]] with ~empty~.

I think that's what we were trying to get at with the original wording here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also prefer adding the phrase the sole occurrence of. I'll make the update as a single commit after above conversation is resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants