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

Fix NPE when regexp search has no result #927

Closed

Conversation

gausie
Copy link
Contributor

@gausie gausie commented Jun 10, 2021

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)

@rbri
Copy link
Collaborator

rbri commented Jun 10, 2021

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

@gausie
Copy link
Contributor Author

gausie commented Jun 10, 2021

Where do I add that test? I'm struggling to navigate the testing setup

@rbri
Copy link
Collaborator

rbri commented Jun 10, 2021

I guess you can use the last method in the class I mentioned in my previous comment as a template

@gausie
Copy link
Contributor Author

gausie commented Jun 10, 2021

@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,

@rbri
Copy link
Collaborator

rbri commented Jun 10, 2021

Many thanks

Copy link
Collaborator

@gbrail gbrail left a 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.

@gausie gausie requested a review from gbrail June 22, 2021 17:39
@gbrail
Copy link
Collaborator

gbrail commented Jun 23, 2021

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!

@gbrail gbrail closed this Jun 23, 2021
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