Editorial: refactor IsStringPrefix and String.prototype.split to use StringIndexOf; remove SplitMatch

[shvaikalesh]

This PR merges IsStringPrefix and SplitMatch abstract ops into single well-defined StringIncludesAt helper, improving:

• unobvious parameter order of IsStringPrefix;
• too specific name and polymorphic return type of SplitMatch.

We can't use StringIndexOf instead, since it searches until the end of string rather than matching substring only at index.

[michaelficarra]

For readability, we could leave this with an implementation that just passes 0 as the third parameter to StringIncludesAt. Not sure if it's worth it.

[shvaikalesh]

It's maybe just for me, but parameter order of IsStringPrefix isn't immediately clear: is first parameter a string or a prefix?

[michaelficarra]

I feel the same way about StringIncludesAt.

[shvaikalesh]

It would be great if we came up with a better name. StringMatch?

[michaelficarra]

I don't really think the name will ever be sufficient. I will always need to take a quick look at the parameter names or the summary. Or infer it from context at the usage site.

[ljharb]

i don't find this change clearer :-/

[ljharb]

since the "else" is multiline, i think the "if" needs to be too.

[jmdyck]

The spec has about 37 examples of an 'inline' If followed by a 'multiline' Else. And about 18 examples of a multiline If followed by an inline Else.

[ljharb]

Compared to how many examples are both multiline or both inline, those seem like cases we should make consistent.

[ljharb]

Discussed this on the editor call; let's please leave IsStringPrefix and its callsites untouched (but update its implementation to call into StringIncludesAt). I'm not personally convinced that StringIncludesAt is the best name but don't have a better suggestion at this time.

[gibson042]

I'm in favor of removing redundant operations, but not clear on why it's necessary to introduce a new one rather than using StringIndexOf. IsStringPrefix(_possiblePrefix_, _string_) is equivalent to StringIndexOf(_string_, _possiblePrefix_, 0) is 0, and String.prototype.split can be refactored as well:

1. Let _s_ be the length of _S_.
1. Let _separatorLength_ be the length of _R_.
1. If _separatorLength_ is 0, then
  1. Let _head_ be the substring of _S_ from 0 to _lim_.
  1. Let _codeUnits_ be a List consisting of the sequence of code units that are the elements of _head_.
  1. Return ! CreateArrayFromList(_codeUnits_).
1. If _s_ is 0, return ! CreateArrayFromList(« _S_ »).
1. Let _substrings_ be a new empty List.
1. Let _i_ be 0.
1. Let _j_ = StringIndexOf(_S_, _R_, 0).
1. Repeat, while _j_ is not -1,
  1. Let _T_ be the substring of _S_ from _i_ to _j_.
  1. Append _T_ to _substrings_.
  1. Set _lengthA_ to _lengthA_ + 1.
  1. If _lengthA_ = _lim_, return ! CreateArrayFromList(_substrings_).
  1. Set _i_ to _j_ + _separatorLength_.
  1. Set _j_ to StringIndexOf(_S_, _R_, _i_).
1. Let _T_ be the substring of _S_ from _i_.
1. Append _T_ to _substrings_.
1. Return ! CreateArrayFromList(_substrings_).

[gibson042]

Suggested change

	<emu-clause id="sec-stringincludesat" aoid="StringIncludesAt">
	<emu-clause id="sec-stringincludesat" aoid="StringIncludesAt" oldids="IsStringPrefix,SplitMatch">

[ljharb]

I've rebased this; the first commit implements #2144 (comment); the second commit reverts that and implements #2144 (review), per one editor's preference on Matrix.

Happy to drop the second, or squash the two, as preferred.

The only unaccounted-for piece in the second commit is adding an oldid somewhere for SplitMatch - I'm not sure where it would go.

updated

[bakkot]

LGTM. The new version of split is much clearer. The old one is opaque enough that it's hard for me to be 100% certain they're identical, but I'm pretty confident they are.