Skip to content
This repository was archived by the owner on Apr 14, 2021. It is now read-only.

Shorten restore regexes #201

Merged
merged 6 commits into from
Jul 21, 2016
Merged

Conversation

mtlewis
Copy link
Contributor

@mtlewis mtlewis commented Jul 5, 2016

Fixes #196.

@nosilleg and I spent some time on this today, and incorporated suggestions from #196 to improve the way createRegExpRestore works. The ability to disable cache/restore of regexes is still there, but the regexes generated now use [\s\S]{n} in conjunction with lastIndex and leftContext to make the generated regexes dramatically shorter.

Mike Lewis added 5 commits July 7, 2016 14:33
The implementation of cache/restore of RegExp properties
has the potential to cause a number of issues.  A perfect
solution is not possible because it's not possible to
determine the regex that was used to turn the cached input
into the cached match and group matches.  The current
solution simply takes the lastMatch and escapes it,
wrapping any match groups in parens.  One problem with
this approach is that when the match is very long, it
will overrun the maximum length of a RegEx in JavaScript.

In this commit, we add the capability to disable restore
of RegExp properties entirely, via
`IntlPolyfill.__disableRegExpRestore`.
Previously createRegExpRestore build a regex with the exact characters for each part of the expression (either capturing group, or segment between capturing groups).  The new solution begins with this approach, and then shortens the resulting expression by replacing each segment with a match for character count only, then setting lastIndex on the resultant RegExp to ensure that the match occurs at the expected location in the input.
createRegExpRestore now returns a restore function rather than an
object that must be operated on.  This commit updates the usage
of this function to line up with the new implementation
@mtlewis mtlewis force-pushed the shorten-restore-regexes branch from 1c59191 to fdaeddb Compare July 7, 2016 13:46
@mtlewis
Copy link
Contributor Author

mtlewis commented Jul 7, 2016

Updated to fix conflicts. There's currently a failing test in master which fails here, too.

@caridy
Copy link
Collaborator

caridy commented Jul 7, 2016

This is looking good @mtlewis. What about inverting the default behavior? and just exposing a method that can be called to enable compliance with spec? This will also have to be called when running test262, but I can take care of that after you change this.

@mtlewis
Copy link
Contributor Author

mtlewis commented Jul 8, 2016

Thanks @caridy!

Wouldn't disabling cache/restore by default constitute a breaking change, and therefore imply a major version bump? It seems like the pragmatic way to get these improvements out to the largest group of people is to:

  1. First merge these changes as they are now, leaving cache/restore on by default and bumping the minor version, then
  2. Switch off cache/restore by default, and bump the major version.

@mtlewis mtlewis force-pushed the shorten-restore-regexes branch from 3573a44 to b70852b Compare July 12, 2016 17:36
@caridy caridy merged commit b70852b into andyearnshaw:master Jul 21, 2016
@caridy
Copy link
Collaborator

caridy commented Jul 21, 2016

@mtlewis thanks for the PR, this is now merged... please try it and let us know :)

@mtlewis
Copy link
Contributor Author

mtlewis commented Jul 22, 2016

Fantastic! Glad to hear it's merged - was a pleasure contributing :)

@mtlewis mtlewis deleted the shorten-restore-regexes branch July 22, 2016 13:56
@caridy
Copy link
Collaborator

caridy commented Sep 20, 2016

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants