Skip to content
This repository has been archived by the owner on Jun 7, 2022. It is now read-only.

Amend specification to use 'd' flag for indices. #49

Merged
merged 5 commits into from
Jan 15, 2021
Merged

Amend specification to use 'd' flag for indices. #49

merged 5 commits into from
Jan 15, 2021

Conversation

rbuckton
Copy link
Collaborator

@rbuckton rbuckton commented Nov 30, 2020 •
edited
Loading

This changes the proposal to only include indices if a d flag is present in the RegExp as a performance mitigation.

NOTE: We have not yet decided as to whether this is the final approach we will take, but this will give us an outline of the specification text we might use to achieve the mitigation.

Preview can be found here: https://tc39.es/proposal-regexp-match-indices/pr/49/index.html

Fixes #46
Fixes #47

@rbuckton rbuckton mentioned this pull request Nov 30, 2020
Closed
ljharb
ljharb suggested changes Dec 1, 2020
View reviewed changes
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

I believe this is missing an entry in “RegExp.prototype.flags”

spec/sec-properties-of-the-regexp-prototype-object-patch.html Outdated Show resolved Hide resolved
spec/sec-properties-of-the-regexp-prototype-object-patch.html Outdated Show resolved Hide resolved
@rbuckton
Copy link
Collaborator Author

@ljharb I've made your requested changes. Can you take another look?

ljharb
ljharb approved these changes Jan 13, 2021
View reviewed changes
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

LGTM pending the one comment

spec/sec-properties-of-the-regexp-prototype-object-patch.html Outdated Show resolved Hide resolved
@rbuckton
Copy link
Collaborator Author

@ljharb I've made the final change you proposed as well as unconditionally creating groupNames as well.

@rbuckton
Copy link
Collaborator Author

One more change, the extra hasGroupNames check in MakeArrayIndices was unnecessary because groupNames[i] should only be something other than undefined if hasGroupNames is true.

@jridgewell
Copy link
Member

jridgewell commented Jan 14, 2021 •
edited
Loading

Why didn't we choose o and offsets? I hate it every time I confuse s flag with sticky, which I've done constantly because flag's letter and name's initial have a much stronger correlation in my mind than "choose first non-reserved letter from the name". Now I'm going to confuse dotAll and the d flag. 😭

@rbuckton
Copy link
Collaborator Author

Why didn't we choose o and offsets? [...]

Primarily to reduce spec and implementation churn, and given that there are no web compat concerns with the name indices that we have seen, changing the name of the property at this stage doesn't seem necessary.

@jridgewell
Copy link
Member

given that there are no web compat concerns with the name indices that we have seen, changing the name of the property at this stage doesn't seem necessary.

We've already hit the "implementations require spec changes" requirement for changes in Stage 3. If we're changing the spec at Stage 3, let's fix it entirely. Using offsets/o is a better choice than indices/d.

@rbuckton
Copy link
Collaborator Author

We'd already discussed this here: #47 (comment). I discussed both d/indices and o/offsets with multiple implementers as well. Another reason we chose to stay with indices is parity with lastIndex and match.index. The term offsets doesn't align with that nomenclature (which is one of the reasons the proposal changed from "offsets" to "indices" back during the stage-2 timeframe).

@jridgewell
Copy link
Member

I had read the other threads, but hadn't seen match.index and regex.lastIndex mentioned in them. Ugh, inconsistencies everywhere!

@rbuckton rbuckton merged commit 592c4a9 into master Jan 15, 2021
@ljharb ljharb deleted the d-flag branch January 15, 2021 02:22
This was referenced Jan 15, 2021
Merged
Merged
pull bot pushed a commit to Alan-love/v8 that referenced this pull request Jan 26, 2021
@syg
[regexp] Implement the /d flag for RegExp indices
81e7e2f 
This CL implements the upcoming spec change:
tc39/proposal-regexp-match-indices#49

A new JSRegExpResultWithIndices subclass is introduced with a separate map and
an extra slot for storing the indices. If /d is passed, exec() constructs a
JSRegExpResultWithIndices and eagerly builds indices.

The existing re-execution logic is removed.

Bug: v8:9548
Change-Id: Ic11853e7521017af5e8bd583c7b82bb672821132
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2616873
Commit-Queue: Shu-yu Guo <[email protected]>
Reviewed-by: Jakob Gruber <[email protected]>
Reviewed-by: Toon Verwaest <[email protected]>
Cr-Commit-Position: refs/heads/master@{#72306}
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ljharb ljharb ljharb approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@rbuckton @jridgewell @ljharb