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

Altered commentRx to avoid a RangeError under certain conditions in io.js (this time with better regexes). #12

Closed
wants to merge 4 commits into from

Conversation

greim
Copy link
Contributor

@greim greim commented Feb 20, 2015

Incorporates some of the changes discussed here: #11 (comment)

Also altered it to avoid a RangeError under certain conditions in io.js.
Changed .+ to ..* to be consistent with commentRx, simplified space
character matcher.
@thlorenz
Copy link
Owner

Could you please rebase this on master and squash into one commit?
Thanks.

@thlorenz
Copy link
Owner

Any news on this? I believe with the latest patch we touched a regex but not the one you changed, so it should still be easy to rebase this.

@greim
Copy link
Contributor Author

greim commented Mar 12, 2015

Okay. Rebased on master, fixed a conflict, had to type git rebase --skip, then squash merged all of that back to master. All to fix a regex. But now it's not on this branch anymore. Want me to make a new PR?

@thlorenz
Copy link
Owner

Yes you can just create a new PR. We can review there what you did and pull it in if all is well.

All to fix a regex

Don't belittle that, optimizing the regex makes a huge difference, so I thank you for taking the time and doing so.
If you think about it, this entire module is mainly a glorified wrapper around using that regex ;)

@greim
Copy link
Contributor Author

greim commented Mar 13, 2015

Closing in favor of a simpler PR.

@greim greim closed this Mar 13, 2015
@greim greim deleted the regex-fix branch March 13, 2015 16:18
@dmitry
Copy link

dmitry commented Jun 30, 2015

@greim what was that?

@greim
Copy link
Contributor Author

greim commented Jul 1, 2015

The essence of the fix had been changing .+ to ..* in a regex. My guess was it was triggering an arcane v8 bug which only happened in very specific circumstances. I eventually just re-ordered some adjacent-but-unrelated gulp tasks and the problem went away :)

@dmitry
Copy link

dmitry commented Jul 1, 2015

@greim thanks for the response! I've similarly fixed an issue: demerzel3/karma-sourcemap-loader#18

In my case it happened on every second pass to match the string, I believe it's somehow related to the V8 regex optimizations.

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

Successfully merging this pull request may close these issues.

None yet

3 participants