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

various editorial changes for comparisons #2877

Merged
merged 1 commit into from
Jan 27, 2023
Merged

Conversation

michaelficarra
Copy link
Member

@michaelficarra michaelficarra commented Aug 30, 2022

Extracted from #2821. Fixes #2794.

Editorial choices included in this PR

  • 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"
  • prefer "if a is either b or c" over "if a is b or a is c"; use "any of" in place of "either" when more than 2 options
  • prefer "if a is c and b is c" over "if a and b are both c"
  • presence in a List is tested with _list_ contains _element_ or _list_ does not contain _element_, not _element_ is an element of _list_ or _element_ is in _list_
  • don't say the value of _a_; just use _a_
  • prefer "if a or b" over "if a or if b"; prefer "if a and b" over "if a and if b"
  • prefer "whose _ is _" over "whose _ is that of _"

Other editorial choices (still to do, not as part of this PR)

  • use a/an/a new for creating and referencing values with identity
  • use the for referencing values without identity

spec.html Outdated Show resolved Hide resolved
@michaelficarra michaelficarra force-pushed the comparison-consistency branch 2 times, most recently from 46f92c7 to e64da4d Compare August 31, 2022 18:29
spec.html Outdated Show resolved Hide resolved
@michaelficarra
Copy link
Member Author

FYI I'm done with my review of the spec for this PR. All changes should now be included and rebased on main. Next, I'll go through all the changes and pull out anything not identity/comparison related into separate PRs, then I'll mark this PR as ready for review.

spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated
1. Set _k_ to _k_ + 1.
1. Set _n_ to _n_ + 1.
1. Else,
1. If _srcType_ is _targetType_, then
Copy link
Member Author

Choose a reason for hiding this comment

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

Note to reviewers: I'm just swapping the positive/negative branches here.

spec.html Outdated Show resolved Hide resolved
@michaelficarra
Copy link
Member Author

Okay that was actually easier than I expected. This is ready for review.

@michaelficarra michaelficarra marked this pull request as ready for review September 16, 2022 21:48
@michaelficarra michaelficarra requested a review from a team September 16, 2022 21:51
@michaelficarra michaelficarra added the editor call to be discussed in the next editor call label Sep 19, 2022
Base automatically changed from misc-editorial to main September 21, 2022 01:52
@michaelficarra
Copy link
Member Author

resolved conflicts

@bakkot
Copy link
Contributor

bakkot commented Dec 5, 2022

@syg may not care to review again, since we've talked about all the decisions, but we should check before landing.

@jmdyck
Copy link
Collaborator

jmdyck commented Dec 7, 2022

Re-doing my analysis from Oct 17...


Composing a condition from sub-conditions...

... when there are only 2 sub-conditions:

  229  {COND} and {COND}
   80  {COND} or {COND}
    4  {COND} unless {COND}
vs
    2  {COND}, or if {COND}
    1  {COND}, or {COND}
    1  {COND}, unless {COND}
vs
    1  either {COND} or {COND}

... when there are 3+ sub-conditions:

    1  {COND}, {COND}, or {COND}
    8  {COND}, {COND}, and {COND}
    6  {COND}, {COND}, {COND}, and {COND}
vs
    5  {COND} and {COND} and {COND}
    1  {COND} or {COND} or {COND}
    1  {COND} or {COND} or {COND} or {COND}

(And while I'm here, conditions that mix operators without parens:

    1  {COND} and {COND} or {COND} and {COND}
    3  {COND} and {COND}, or if {COND} and {COND}
    2  {COND} and {COND}, or {COND} and {COND}
    2  {COND} or {COND} <ins>and {COND}</ins>
    1  {COND} unless {COND} and {COND}
    2  {COND}, or if {COND} and {COND}

)


Testing whether or not a value matches some list of descriptions (e.g., _x_ is *undefined* or a String)...

... when there are only 2 descriptions:

  189  {D} or {D}
vs
  376  either {D} or {D}
vs
   26  either {D}, or {D}

... when there are 3+ descriptions:

    8  any of {D}, {D}, or {D}
    6  any of {D}, {D}, {D}, or {D}
    4  any of {D}, {D}, {D}, {D}, or {D}
    1  any of {D}, {D}, {D}, {D}, {D}, or {D}
    1  any of {D}, {D}, {D}, {D}, {D}, {D}, or {D}
    1  any of {D}, {D}, {D}, {D}, {D}, {D}, {D}, or {D}
vs
    6  either {D}, {D}, or {D}
    5  either {D}, {D}, {D}, or {D}
vs
    3  one of {D}, {D}, or {D}
    1  one of {D}, {D}, {D}, or {D}
vs
   33  {D}, {D}, or {D}
    7  {D}, {D}, {D}, or {D}
    2  {D}, {D}, {D}, {D}, or {D}
    2  {D}, {D}, {D}, {D}, {D}, or {D}
    1  {D}, {D}, {D}, {D}, {D}, {D}, {D}, {D}, {D}, {D}, {D}, or {D}

Copy link
Collaborator

@jmdyck jmdyck left a comment

Choose a reason for hiding this comment

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

Some things I didn't put into PR #2970.

spec.html Outdated
@@ -35337,7 +35343,7 @@ <h1>
1. Assert: _c_ is a MatcherContinuation.
1. Let _Input_ be _x_'s _input_.
1. Let _e_ be _x_'s _endIndex_.
1. If _e_ = 0, or if _rer_.[[Multiline]] is *true* and the character _Input_[_e_ - 1] is one of |LineTerminator|, then
1. If _e_ = 0, or if _rer_.[[Multiline]] is *true* and the character _Input_[_e_ - 1] is matched by |LineTerminator|, then
Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree that is one of |LineTerminator| should be changed, but this use of "is matched by" is odd. Normally, "X is matched by |Foo|" means that:

  • X is a chunk of source text, and
  • X (or, more likely, some larger source text that contains X) has been parsed according to a grammar, resulting in a |Foo| Parse Node that corresponds to X.

But neither of those points is true here.

Maybe:

Suggested change
1. If _e_ = 0, or if _rer_.[[Multiline]] is *true* and the character _Input_[_e_ - 1] is matched by |LineTerminator|, then
1. If _e_ = 0, or if _rer_.[[Multiline]] is *true* and the character _Input_[_e_ - 1] is one of U+000A (LINE FEED), U+000D (CARRIAGE RETURN), U+2028 (LINE SEPARATOR), or U+2029 (PARAGRAPH SEPARATOR), then

spec.html Outdated
@@ -35352,7 +35358,7 @@ <h1>
1. Let _Input_ be _x_'s _input_.
1. Let _e_ be _x_'s _endIndex_.
1. Let _InputLength_ be the number of elements in _Input_.
1. If _e_ = _InputLength_, or if _rer_.[[Multiline]] is *true* and the character _Input_[_e_] is one of |LineTerminator|, then
1. If _e_ = _InputLength_, or if _rer_.[[Multiline]] is *true* and the character _Input_[_e_] is matched by |LineTerminator|, then
Copy link
Collaborator

Choose a reason for hiding this comment

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

(ditto previous comment)

spec.html Outdated
@@ -35916,7 +35922,7 @@ <h1>
<emu-alg>
1. Let _basicWordChars_ be the CharSet containing every character in the ASCII word characters.
1. Let _extraWordChars_ be the CharSet containing all characters _c_ such that _c_ is not in _basicWordChars_ but Canonicalize(_rer_, _c_) is in _basicWordChars_.
1. Assert: _extraWordChars_ is empty unless _rer_.[[Unicode]] and _rer_.[[IgnoreCase]] are both *true*.
1. Assert: _extraWordChars_ is empty unless _rer_.[[Unicode]] is *true* and _rer_.[[IgnoreCase]] is *true*.
Copy link
Collaborator

Choose a reason for hiding this comment

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

There are several remaining occurrences of "X and Y are both Z", so it's unclear why this particular one would be changed.

spec.html Outdated Show resolved Hide resolved
@bakkot
Copy link
Contributor

bakkot commented Dec 7, 2022

I don't love the ToIntegerOrInfinity -> ToUint32 change, because ToUint32 has wrapping behavior. It happens that the wrapping behavior doesn't come up in this case because array indices are bounded below the modulus, but I don't like making the reader think that hard.

Can I suggest ! ToNumber(_P_) ≥ _newLen_ instead? I would also be fine with StringToNumber(_P_) ≥ _newLen_, or with ! ToIntegerOrInfinity(_P_) ≥ ℝ(_newLen_).

@michaelficarra michaelficarra removed the editor call to be discussed in the next editor call label Dec 7, 2022
@jmdyck
Copy link
Collaborator

jmdyck commented Dec 8, 2022

I don't love the ToIntegerOrInfinity -> ToUint32 change, because ToUint32 has wrapping behavior. It happens that the wrapping behavior doesn't come up in this case because array indices are bounded below the modulus, but I don't like making the reader think that hard.

Note that there's a call to ToUint32(_P_) just 3 lines down, so they're going to have to think that hard anyway. (On the other hand, maybe that call should change too. Not in this PR necessarily.)

Can I suggest ! ToNumber(_P_) ≥ _newLen_ instead? I would also be fine with StringToNumber(_P_) ≥ _newLen_, or with ! ToIntegerOrInfinity(_P_) ≥ ℝ(_newLen_).

Those all work. The algorithm doesn't currently use (or any mathematical values at all?), so I think it's easier on the reader if we don't use the third alternative. Between the other two, I have a mild preference for StringToNumber(_P_). (The name is more explicit, it's a simpler AO, and avoiding ! in a condition is a bonus.)

@michaelficarra michaelficarra force-pushed the comparison-consistency branch 2 times, most recently from 58a0f40 to 40fc334 Compare January 25, 2023 23:54
@michaelficarra michaelficarra added the ready to merge Editors believe this PR needs no further reviews, and is ready to land. label Jan 25, 2023
Co-authored-by: Michael Dyck <jmdyck@ibiblio.org>
Co-authored-by: Michael Ficarra <mficarra@shapesecurity.com>
@ljharb ljharb merged commit f1fbcc5 into main Jan 27, 2023
@ljharb ljharb deleted the comparison-consistency branch January 27, 2023 17:15
jmdyck added a commit to jmdyck/ecma262 that referenced this pull request Jul 28, 2023
This one is debatable.
I mostly did it for my convenience,
but I think PR tc39#2877 also preferred some similar expansions.
syg pushed a commit to syg/ecma262 that referenced this pull request Aug 23, 2023
This one is debatable.
I mostly did it for my convenience,
but I think PR tc39#2877 also preferred some similar expansions.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
editorial change establishes editorial conventions 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.

use consistent phrasing for comparing string values
6 participants