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: remove some unnecessary calls to SameValue #3202

Merged
merged 1 commit into from
Oct 19, 2023

Conversation

michaelficarra
Copy link
Member

I'm sure there are more of these, these are the ones I noticed today.

@jmdyck
Copy link
Collaborator

jmdyck commented Oct 17, 2023

There's one other place where SameValue is used to compare two Boolean values, in [[IsExtensible]] for a Proxy exotic object.

@jmdyck
Copy link
Collaborator

jmdyck commented Oct 17, 2023

More generally, what's the editorial convention for using SameValue vs is?

E.g., it might be something like...

When comparing two values for (something like) equality:

  • If either value might be a specification value, then you can't just use SameValue, because it only accepts ECMAScript language values.
  • If both values might be Numbers, then you must use SameValue, because you want the specificity of Number::sameValue.
  • Otherwise (i.e., if both values are ECMAScript language values, but not both Numbers), then you could use either SameValue or is, but the preference is to use is.

@michaelficarra
Copy link
Member Author

michaelficarra commented Oct 17, 2023

@jmdyck From #2877:

  • use =, , <, , >, and for mathematical value and bigint comparisons (except in asserts); prefer = over
  • use is, is not, <, , >, and for number comparisons
  • use SameValue(..., ...) is *true* for equality comparison of symbols (except well-known) / objects / unknown ECMAScript language values
  • use is and is not for equality comparison of all other values, including booleans, strings, well-known symbols, null, undefined, enums, etc; avoid "is different from" or "is the same as"

@michaelficarra
Copy link
Member Author

There's one other place where SameValue is used to compare two Boolean values

Added.

@michaelficarra michaelficarra added the ready to merge Editors believe this PR needs no further reviews, and is ready to land. label Oct 18, 2023
@ljharb ljharb force-pushed the ValidateAndApplyPropertyDescriptor-SameValue branch from 223c401 to 57d53d3 Compare October 19, 2023 04:29
@ljharb ljharb merged commit 57d53d3 into main Oct 19, 2023
7 checks passed
@ljharb ljharb deleted the ValidateAndApplyPropertyDescriptor-SameValue branch October 19, 2023 04:42
zhangenming pushed a commit to zhangenming/ecma262 that referenced this pull request Dec 22, 2023
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.

4 participants