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

Some fixes and cleanup #27

Merged
merged 2 commits into from Aug 1, 2019

Conversation

@rbuckton
Copy link
Collaborator

commented Jul 26, 2019

While drafting the PR for tc39/ecma262 I notices a few issues in the proposal that I'd like to have reviewed:

  • In BackreferenceMatcher: _r_._startIndex_ and _r_._endIndex_ do not seem to be the correct way to reference these values.
  • In RegExpBuiltinExec: There was a leftover reference to _matchOptions_ that had not been removed.
  • In RegExpBuiltinExec: _indices_ was still incorrectly defined as a Record when it should have been a List.
  • Cleaned up Asserts in GetStringIndex, GetMatchString, GetMatchIndicesArray
  • In MakeGetIndices: The "groups" property on the "indices" array result is not being created in the same way as the "groups" property on the result of RegExpBuiltinExec:
    • result.groups always exists if there is a GroupName production in the pattern, but result.indices.groups only exists if there was a successful capture in a named capture group.
    • result.groups[name] always exists (but may be undefined) for every GroupName in the pattern, but result.indices.groups[name] only exists if there was a successful capture in a named capture group.
    • Addressed this by providing the group names for each possible capture group as a [[GroupNames]] internal slot on the "indices" getter and removing the [[GroupName]] field from the Match Record.
  • In MakeGetIndices: The loop used the prose the _i_<sup>th</sup> element of _indices_, which is incorrect and introduces an off-by-one error as the spec stats that the _i_th element of _x_ is equivalent to _x_[_i_ - 1] (i.e. The 1st element of _x_ refers to _x_[0]).

Rendered Spec: https://tc39.es/proposal-regexp-match-indices/pr/27

@rbuckton

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 26, 2019

You can preview the updated version here: https://tc39.es/proposal-regexp-match-indices/pr/27/

Apply suggestions from code review
Co-Authored-By: Jordan Harband <ljharb@gmail.com>
@rbuckton

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 1, 2019

@ljharb any other feedback?

@ljharb

ljharb approved these changes Aug 1, 2019

@rbuckton rbuckton merged commit d169f29 into master Aug 1, 2019

1 check passed

tc39.proposal-regexp-match-indices Build #20190730.1 succeeded
Details

@rbuckton rbuckton deleted the proposalCleanup branch Aug 1, 2019

@rbuckton

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 1, 2019

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.