-
Notifications
You must be signed in to change notification settings - Fork 863
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
Fix NPE when regexp search has no result #927
Conversation
It will be great if you can add a test method that shows the problem to rhino/testsrc/org/mozilla/javascript/tests/es6/NativeRegExpTest.java |
Where do I add that test? I'm struggling to navigate the testing setup |
I guess you can use the last method in the class I mentioned in my previous comment as a template |
@rbri ah, I misread your first message and thought that you were linking me to the source file not the test! Thank you and done - the attached test fails without my change, |
Many thanks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks -- looks mostly great, but I'd like you to look at the handling of the Context object in the text -- see the comment below.
Thanks for doing this work. I ended up merging and squashing the other version of this PR because both are basically the same. However I just realized that because I "squashed" the PR into one commit, so your name doesn't show up in the commit history directly (although it's in a comment). I'll check more carefully next time -- I want to make sure that we have a diversity of contributors and that people can use work on this project on their "GitHub resume" so let us know if you have other changes to make! |
Not sure if I need to do anything tests-wise for this (and indeed, how to do that) but I ran into this NPE when adding rhino's compatibility data to
core-js
(zloirock/core-js#942)