New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Normative: add set methods #3306
base: main
Are you sure you want to change the base?
Conversation
Rather than suggest changes here, I've filed a PR against the set-methods branch: #3308 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In isDisjointFrom, there's a "Set thisSize to the number of elements in O.[[SetData]]." line, but there's a bunch of other places that use SetDataSize. I see the difference - the AO skips empty values, the referenced line does not - but is that a problem? it seems like they should maybe all skip empty values?
<emu-alg> | ||
1. If _obj_ is not an Object, throw a *TypeError* exception. | ||
1. Let _rawSize_ be ? Get(_obj_, *"size"*). | ||
1. Let _numSize_ be ? ToNumber(_rawSize_). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this (and the below ToIntegerOrInfinity coercion) conflict with your "no coercion" initiative?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, but there wasn't much appetite to retroactively apply that to existing stage 3 proposals.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I was you, I would bring this one up. It's not like it will be hard or risky to do at this point.
Not a problem - the subsequent lines explicitly handle |
</tr> | ||
<tr> | ||
<td> | ||
[[Set]] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is going to make Ctrl+F-ing for [[Set]]
even more annoying. Maybe [[SetLike]]
or [[SetObject]]
would be more appropriate anyway?
1. NOTE: Because _other_ is an arbitrary object, it is possible for its *"keys"* iterator to produce the same value more than once. | ||
1. Let _alreadyInResult_ be SetDataHas(_resultSetData_, _next_). | ||
1. Let _inThis_ be SetDataHas(_O_.[[SetData]], _next_). | ||
1. If _alreadyInResult_ is *false* and _inThis_ is *true*, then | ||
1. Append _next_ to _resultSetData_. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it'd be better to more closely mirror the other branch above. Also, this is probably how implementations will do it anyway for efficiency purposes:
1. NOTE: Because _other_ is an arbitrary object, it is possible for its *"keys"* iterator to produce the same value more than once. | |
1. Let _alreadyInResult_ be SetDataHas(_resultSetData_, _next_). | |
1. Let _inThis_ be SetDataHas(_O_.[[SetData]], _next_). | |
1. If _alreadyInResult_ is *false* and _inThis_ is *true*, then | |
1. Append _next_ to _resultSetData_. | |
1. Let _inThis_ be SetDataHas(_O_.[[SetData]], _next_). | |
1. If _inThis_ is *true*, then | |
1. NOTE: Because _other_ is an arbitrary object, it is possible for its *"keys"* iterator to produce the same value more than once. | |
1. Let _alreadyInResult_ be SetDataHas(_resultSetData_, _next_). | |
1. If _alreadyInResult_ is *false*, then | |
1. Append _next_ to _resultSetData_. |
1. Let _alreadyInResult_ be SetDataHas(_resultSetData_, _e_). | ||
1. If _alreadyInResult_ is *false*, then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given we have the note above the step, the alias name isn't adding anything, and I think we can inline this.
1. Let _alreadyInResult_ be SetDataHas(_resultSetData_, _e_). | |
1. If _alreadyInResult_ is *false*, then | |
1. If SetDataHas(_resultSetData_, _e_) is *false*, then |
Same below.
1. Let _resultIndex_ be SetDataIndex(_resultSetData_, _next_). | ||
1. If SetDataHas(_O_.[[SetData]], _next_) is *true*, then | ||
1. If _resultIndex_ is not ~not-found~, set _resultSetData_[_resultIndex_] to ~empty~. | ||
1. Else, | ||
1. If _resultIndex_ is ~not-found~, append _next_ to _resultSetData_. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find this easier to read if we introduce an alreadyInResult
alias:
1. Let _resultIndex_ be SetDataIndex(_resultSetData_, _next_). | |
1. If SetDataHas(_O_.[[SetData]], _next_) is *true*, then | |
1. If _resultIndex_ is not ~not-found~, set _resultSetData_[_resultIndex_] to ~empty~. | |
1. Else, | |
1. If _resultIndex_ is ~not-found~, append _next_ to _resultSetData_. | |
1. If SetDataIndex(_resultSetData_, _next_) is ~not-found~, let _alreadyInResult_ be *false*. Otherwise let _alreadyInResult_ be *true*. | |
1. If SetDataHas(_O_.[[SetData]], _next_) is *true*, then | |
1. If _alreadyInResult_ is *true*, set _resultSetData_[_resultIndex_] to ~empty~. | |
1. Else, | |
1. If _alreadyInResult_ is *false*, append _next_ to _resultSetData_. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM otherwise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since "set" is a verb—and an especially common one in this specification at that, frequently paired with "get"—the new operation names strike me as a bit cumbersome (e.g., "SetDataHas" reads like "set the 'dataHas' of something" at first blush, rather then the "check if setData has a value" actually describing it). It would be nice to avoid such confusion by renaming them to avoid a leading "set" noun. Suggestions:
- SetDataHas → IsInSetData
- SetDataIndex → IndexInSetData
- SetDataSize → SizeOfSetData
I am aware that this deviates from the convention of starting operation names with the type of their first parameter, but a) that convention is not universal, cf. Operations on Objects, and b) the benefits of deviation seem to outweigh the drawbacks in this case.
<emu-clause id="sec-getsetrecord" type="abstract operation"> | ||
<h1> | ||
GetSetRecord ( | ||
_obj_: an ECMAScript language value, | ||
): either a normal completion containing a Set Record or a throw completion |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Get" seems like an inappropriate prefix for an operation that constructs something new. Suggestion:
<emu-clause id="sec-getsetrecord" type="abstract operation"> | |
<h1> | |
GetSetRecord ( | |
_obj_: an ECMAScript language value, | |
): either a normal completion containing a Set Record or a throw completion | |
<emu-clause id="sec-createsetrecord" type="abstract operation"> | |
<h1> | |
CreateSetRecord ( | |
_obj_: an ECMAScript language value, | |
): either a normal completion containing a Set Record or a throw completion |
(cf. CreateListIteratorRecord and Temporal Create*Record methods like CreateDurationRecord for this pattern, and also Make{DataView,TypedArray}WithBufferWitnessRecord and other "Make" operations or even "New" operations like NewObjectEnvironment for alternatives, although I think "Make" is more often used to create language values and "New" for environment records. I would also like to address existing uses of the "Get" name like GetIteratorFromMethod, but at least this PR can avoid the bad precedent)
1. Repeat, while _next_ is not ~done~, | ||
1. Set _next_ to ? IteratorStepValue(_keysIter_). | ||
1. If _next_ is not ~done~, then | ||
1. If _next_ is *-0*<sub>𝔽</sub>, set _next_ to *+0*<sub>𝔽</sub>. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should consider abstracting this into its own operation; it's too subtle to see it duplicated without clear identification of purpose.
1. If _next_ is *-0*<sub>𝔽</sub>, set _next_ to *+0*<sub>𝔽</sub>. | |
1. Set _next_ to CanonicalizeCollectionKey(_next_). |
See proposal.