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.

Sign up for GitHub

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

Jump to bottom

Don't allocate the sticky regex source when the resulting regexp is already cached #154

Merged
merged 2 commits into from
Feb 12, 2017
Merged

Don't allocate the sticky regex source when the resulting regexp is already cached #154

merged 2 commits into from
Feb 12, 2017

Conversation

pygy
Copy link
Contributor

@pygy pygy commented Feb 12, 2017

As discussed previously in #152 and #153

slevithan
slevithan reviewed Feb 12, 2017
View reviewed changes
src/xregexp.js Outdated
@@ -839,7 +839,7 @@ XRegExp.exec = function(str, regex, pos, sticky) {
regex[REGEX_DATA][cacheKey] = copyRegex(regex, {
addG: true,
addY: addY,
source: sourceY,
source: fakeY? regex.source + '|()' : void 0,
Copy link
Owner

Choose a reason for hiding this comment

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

Nits: Space before ?, and please just use undefined rather than void 0.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, updating

@pygy
Copy link
Contributor Author

pygy commented Feb 12, 2017

Thinking more about this, I'm not sure what to do for regexps that are both global and sticky. It makes little sense to set both flags, but it is legal AFAIK... Scaling down.

@slevithan
Copy link
Owner

To be clear, the "fast sticky polyfill" rev is just avoiding an unnecessary string concat in some cases, and otherwise not changing behavior at all, correct?

@pygy pygy changed the title Don't allocate the sticky regex source when the resulting regexp is already cached and reflect lastIndex on sticky regexps Don't allocate the sticky regex source when the resulting regexp is already cached Feb 12, 2017
@pygy
Copy link
Contributor Author

pygy commented Feb 12, 2017 •
edited
Loading

Correct. I've removed the lastIndex thing, I'll make another PR once the gy situation is more clear.

@slevithan slevithan merged commit 065de70 into slevithan:master Feb 12, 2017
@slevithan
Copy link
Owner

slevithan commented Feb 12, 2017 •
edited
Loading

I've removed the lastIndex thing, I'll make another PR once the gy situation is more clear.

Thanks--perfect! Removing it from this PR made me more confident to merge the other change.

BTW, the reason XRegExp isn't yet fully tackling lastIndex setting for /y is because much of the related code was written before the existence of ES6 /y. It was based on /y in old versions of Firefox. I appreciate you tackling this to bring XRegExp up to spec with ES6 lastIndex handling for /y and /gy.

@slevithan
Copy link
Owner

@pygy BTW, if you submit robust /y + lastIndex handling, I'd be happy to publish a v3.1.2 that gets your /y speed and correctness updates out there sooner.

@pygy
Copy link
Contributor Author

pygy commented Feb 15, 2017

Ok, According to both the latest draft and the ES6 spec, y takes precedence over g. I'll look into it tomorrow evening.

@pygy pygy mentioned this pull request Feb 16, 2017
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@slevithan slevithan slevithan left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@pygy @slevithan