Skip to content
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

Editorial: introduce CanonicalizeKeyedCollectionKey #3337

Merged
merged 1 commit into from
May 31, 2024

Conversation

bakkot
Copy link
Contributor

@bakkot bakkot commented May 27, 2024

Copy link
Member

@michaelficarra michaelficarra left a comment

Choose a reason for hiding this comment

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

Oooh, nice!

@ljharb ljharb requested a review from a team May 28, 2024 06:45
Copy link
Contributor

@gibson042 gibson042 left a comment

Choose a reason for hiding this comment

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

Thanks! This could also be used by the related algorithms that currently rely upon SameValueZero.

           1. For each element _e_ of …, do
-            1. If _e_ is not ~empty~ and SameValueZero(_e_, _value_) is *true*, …
+            1. If _e_ is not ~empty~ and CanonicalizeKeyedCollectionKey(_value_) is _e_, …

or

           1. For each element _e_ of …, do
-            1. If _e_ is not ~empty~ and SameValueZero(_e_, _value_) is *true*, …
+            1. If CanonicalizeKeyedCollectionKey(_value_) is _e_, …

@ljharb
Copy link
Member

ljharb commented May 28, 2024

I like this AO, but that specific suggestion seems like it'd reduce clarity to me - i'd think it'd be better to retain separate "canonicalize" and "comparison" AOs.

@michaelficarra
Copy link
Member

@gibson042 I certainly wouldn't want to replace all uses of SameValueZero, but I'd be fine doing that for the ones in Map/Set.

@bakkot bakkot added the editor call to be discussed in the next editor call label May 29, 2024
@bakkot
Copy link
Contributor Author

bakkot commented May 30, 2024

This could also be used by the related algorithms that currently rely upon SameValueZero.

Done, though I've written it as canonicalizing before the loop rather than in every step of the loop.

It's technically slightly redundant to write If _e_ is not ~empty~ and _e_ is _value_ because _value_ is never ~empty~, but I've chosen to keep the first half to emphasize that holes are being skipped here.

@michaelficarra michaelficarra added ready to merge Editors believe this PR needs no further reviews, and is ready to land. and removed editor call to be discussed in the next editor call labels May 30, 2024
@ljharb ljharb force-pushed the canonicalize-collection-index branch from 7fee271 to 30257dd Compare May 30, 2024 23:58
@ljharb ljharb merged commit 30257dd into main May 31, 2024
8 checks passed
@ljharb ljharb deleted the canonicalize-collection-index branch May 31, 2024 00:01
jmdyck added a commit to jmdyck/ecma262 that referenced this pull request May 31, 2024
(apply editorial convention to recently-merged PR tc39#3337)
ljharb pushed a commit to jmdyck/ecma262 that referenced this pull request Jun 26, 2024
…3340)

(apply editorial convention to recently-merged PR tc39#3337)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
editorial change ready to merge Editors believe this PR needs no further reviews, and is ready to land.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants