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: Make better use of clamping #2917

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

gibson042
Copy link
Contributor

  • Reduce single-use aliases.
  • Introduce ToClampedIndex for relative-to-start-or-end index values.

@ljharb
Copy link
Member

ljharb commented Sep 27, 2022

Nice, this looks a lot cleaner to me

@jmdyck
Copy link
Collaborator

jmdyck commented Sep 27, 2022

This has a conflict with main in String.prototype.lastIndexOf.

spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
@gibson042
Copy link
Contributor Author

@jmdyck Thanks, fixed all three issues.

@gibson042 gibson042 requested a review from jmdyck October 3, 2022 02:17
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.

As far as I can tell, this PR is semantics-preserving (or, in one case, semantics-clarifying), and could be merged without calamity.

My comments are stylistic points that you and/or the editors might want to consider.

spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated
Comment on lines 33726 to 33729
1. If _numPos_ is *NaN*, let _start_ be _len_.
1. Else, let _start_ be the result of clamping ! ToIntegerOrInfinity(_numPos_) between 0 and _len_.
1. If _searchStr_ is the empty String, return 𝔽(_start_).
1. For each integer _i_ such that 0 ≤ _i_ ≤ _start_, in descending order, do
1. For each integer _i_ such that 0 ≤ _i_ ≤ min(_start_, _len_ - _searchLen_), in descending order, do
Copy link
Collaborator

Choose a reason for hiding this comment

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

If the search-string is longer than the subject-string (i.e., _len_ - _searchLen_ < 0), the status quo calls for the result of clamping _pos_ between 0 and a negative number, which violates clamping's requirement that lower <= upper.

This PR fixes that spec bug, which I think deserves pointing out.

spec.html Outdated
1. Let _k_ be _relativeIndex_.
1. Else,
1. Let _k_ be _len_ + _relativeIndex_.
1. Let _k_ be ? ToClampedIndex(_index_, _len_, -1).
Copy link
Collaborator

Choose a reason for hiding this comment

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

In

  • String.prototype.at
  • Array.prototype.at
  • %TypedArray%.prototype.at

it's odd to use ToClampedIndex, because the given clamping is superfluous -- the subsequent code works whether the quantity has been clamped or not. Instead, what's important is that negative-indexing-from-end has been applied.

We could conceivably introduce an AO that performed only that transformation.
(see HandleNegativeIndex above). Other functions that could maybe use it:

  • Array.prototype.includes
  • Array.prototype.indexOf
  • %TypedArray%.prototype.includes
  • %TypedArray%.prototype.indexOf

Copy link
Contributor Author

@gibson042 gibson042 Oct 6, 2022

Choose a reason for hiding this comment

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

I tried to find a name that conveyed enough information to justify use in both …indexOf and at operations, and ultimately decided that including "clamp" was important to understand the arguments ("To" implies conversion, "Clamped" implies relevance of bounds, and "Index" loosely suggests handling of negative values, especially as "ClampedIndex").

ToClampedRelativeIndex was a possibility, but it's a bit long and also seemed a little weird to refer to nonnegative indices as "relative". Likewise ToClampedAbsoluteIndex, which seems like just a longer name for ToClampedIndex.

spec.html Outdated
1. Let _k_ be min(_n_, _len_ - 1).
1. Else,
1. Let _k_ be _len_ + _n_.
1. If _fromIndex_ is not present, let _k_ be _len_ - 1; else let _k_ be ? ToClampedIndex(_fromIndex_, _len_, -1, _len_ - 1).
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
1. If _fromIndex_ is not present, let _k_ be _len_ - 1; else let _k_ be ? ToClampedIndex(_fromIndex_, _len_, -1, _len_ - 1).
1. If _fromIndex_ is not present, then
1. Let _k_ be _len_ - 1.
1. Else,
1. Let _k_ be ? ToClampedIndex(_fromIndex_, _len_, -1, _len_ - 1).

just for readability. And similarly for other long If/else steps.

spec.html Outdated Show resolved Hide resolved
@bakkot bakkot added the editor call to be discussed in the next editor call label Oct 9, 2022
spec.html Outdated Show resolved Hide resolved
spec.html Outdated
Comment on lines 37656 to 37657
1. If _fromIndex_ is not present, let _k_ be _len_ - 1.
1. Else, let _k_ be min(? ToAbsoluteIndex(_fromIndex_, _len_), _len_ - 1).
Copy link
Member

Choose a reason for hiding this comment

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

Any reason not to make this a positive test?

Suggested change
1. If _fromIndex_ is not present, let _k_ be _len_ - 1.
1. Else, let _k_ be min(? ToAbsoluteIndex(_fromIndex_, _len_), _len_ - 1).
1. If _fromIndex_ is present, let _k_ be min(? ToAbsoluteIndex(_fromIndex_, _len_), _len_ - 1).
1. Else, let _k_ be _len_ - 1.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not particularly, but "If <input> is <negative condition>, let <position> be <maximum>… else let <position> be <treat input numerically>" is the overwhelmingly dominant pattern in such steps of the algorithms affected by this PR (although {Array,%TypedArray%}.prototype.lastIndexOf are unique in their condition distinguishing absence from undefined1).

How strongly do you feel about this suggestion?

Footnotes

  1. …and String.prototype.lastIndexOf is unique in conflating non-numeric end-position input with absent/undefined, as opposed to e.g. slice and endsWith and subarray 🙃

@bakkot bakkot removed the editor call to be discussed in the next editor call label Oct 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants