Editorial: update for String{,Last}IndexOf returning not-found

[bakkot]

Update for tc39/ecma262#3300. Marked as draft until that lands; I will update this PR to pull in the extra biblio when it does (though the type-checking isn't yet able to do anything with that). I've pulled in the biblio for that PR.

I'm not absolute sure about the changes PartitionPattern and PartitionNotationSubPattern and would appreciate extra scrutiny there.

[bakkot]

Updated the biblio; this is ready for review/landing.

[ben-allen]

This looks entirely good to me.

Note, though: As a result of changes made in #840, merging the changes in PartitionPattern with the current main is somewhat tricky. I've rebased it on a branch in my own fork, but getting another set of eyes on it would be prudent. Here's a direct link to the relevant AO
@gibson042 @bakkot

[bakkot]

@ben-allen looks like _placeholderEnd_ still needs updating in the AO you linked

[ben-allen]

@ben-allen looks like _placeholderEnd_ still needs updating in the AO you linked

The Let _placeholderEnd_ be -1 line? That's included (and left unchanged) because _placeholderEnd_ can't be ~not-found~ — as I understand it, whenever there's a { there's guaranteed to be a matching }, and so there's no way inside the Repeat, while _placeholderStart_ is not ~not-found~, loop for _placeholderEnd_ to get set to ~not-found~.

It's set to -1 before the loop and compared to -1 after the loop so that _tail_ doesn't get included in _result_ twice in the case that _pattern_ has no placeholders.

[bakkot]

Ah. In that case it should use a different value than -1, like ~none~, and the 1. Assert: _placeholderStart_ < _placeholderEnd_ line should be changed Assert: _placeholderEnd_ is not ~not-found~ and _placeholderStart_ < _placeholderEnd_.

[ben-allen]

Ah. In that case it should use a different value than -1, like ~none~, and the 1. Assert: _placeholderStart_ < _placeholderEnd_ line should be changed Assert: _placeholderEnd_ is not ~not-found~ and _placeholderStart_ < _placeholderEnd_.

Ah, brilliant — changing now

[ben-allen]

link to updated PartitionPattern

[bakkot]

Ah, hm, it didn't occur to me that this does mess up the first line of the loop.

1. Let _literal_ be the substring of _pattern_ from _placeholderEnd_ + 1 to _placeholderStart_.

I guess using -1 is fine and just updating the assert.

[ben-allen]

Apologies for all the fixups! Here's a rewritten version using ~none~ rather than -1 to initialize _placeholderEnd_. This is slightly messy — I'm sure there's a more elegant way — but perhaps it's less opaque than the -1 version.

[bakkot]

That version of PartitionPattern LGTM, thanks for taking care of that!

[ben-allen]

@bakkot with your permission I'll push what I've got here to your branch and merge.

[bakkot]

Go for it.

[gibson042]

FWIW, I think it would be more readable for placeholderEnd to still be initialized to -1 and remain integer-valued.

1. Let _result_ be a new empty List.
1. Let _placeholderEnd_ be -1.
1. Let _placeholderStart_ be StringIndexOf(_pattern_, *"{"*, 0).
1. Repeat, while _placeholderStart_ is not ~not-found~,
  1. Let _literal_ be the substring of _pattern_ from _placeholderEnd_ + 1 to _placeholderStart_.
  1. If _literal_ is not the empty String, then
    1. Append the Record { [[Type]]: *"literal"*, [[Value]]: _literal_ } to _result_.
  1. Set _placeholderEnd_ to StringIndexOf(_pattern_, *"}"*, _placeholderStart_).
  1. Assert: _placeholderEnd_ is not ~not-found~ and _placeholderStart_ < _placeholderEnd_.
  1. Let _placeholderName_ be the substring of _pattern_ from _placeholderStart_ + 1 to _placeholderEnd_.
  1. Append the Record { [[Type]]: _placeholderName_, [[Value]]: *undefined* } to _result_.
  1. Set _placeholderStart_ to StringIndexOf(_pattern_, *"{"*, _placeholderEnd_).
1. Let _tail_ be the substring of _pattern_ from _placeholderEnd_ + 1.
1. If _tail_ is not the empty String, then
  1. Append the Record { [[Type]]: *"literal"*, [[Value]]: _tail_ } to _result_.
1. Return _result_.

[ben-allen]

FWIW, I think it would be more readable for placeholderEnd to still be initialized to -1 and remain integer-valued.

1. Let _result_ be a new empty List.
1. Let _placeholderEnd_ be -1.
1. Let _placeholderStart_ be StringIndexOf(_pattern_, *"{"*, 0).
1. Repeat, while _placeholderStart_ is not ~not-found~,
  1. Let _literal_ be the substring of _pattern_ from _placeholderEnd_ + 1 to _placeholderStart_.
  1. If _literal_ is not the empty String, then
    1. Append the Record { [[Type]]: *"literal"*, [[Value]]: _literal_ } to _result_.
  1. Set _placeholderEnd_ to StringIndexOf(_pattern_, *"}"*, _placeholderStart_).
  1. Assert: _placeholderEnd_ is not ~not-found~ and _placeholderStart_ < _placeholderEnd_.
  1. Let _placeholderName_ be the substring of _pattern_ from _placeholderStart_ + 1 to _placeholderEnd_.
  1. Append the Record { [[Type]]: _placeholderName_, [[Value]]: *undefined* } to _result_.
  1. Set _placeholderStart_ to StringIndexOf(_pattern_, *"{"*, _placeholderEnd_).
1. Let _tail_ be the substring of _pattern_ from _placeholderEnd_ + 1.
1. If _tail_ is not the empty String, then
  1. Append the Record { [[Type]]: *"literal"*, [[Value]]: _tail_ } to _result_.
1. Return _result_.

Upon consideration, I'm given to agree. My line of reasoning is roughly "Which is more potentially annoying to an implementer? The use of -1 instead of something self-documenting, or the additional tests for ~none~ / weirdness involved in making sure that a pattern without placeholders not get appended to _result_ twice?" and thinking that -1 indeed less annoying.