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

Implement SignalException class #4126

Merged
merged 3 commits into from Sep 15, 2016
Merged

Conversation

etehtsea
Copy link
Contributor

@etehtsea etehtsea commented Sep 1, 2016

Fixes #3954

public IRubyObject initialize(IRubyObject[] args, Block block) {
final Ruby runtime = getRuntime();
int argnum = 1;
IRubyObject sig = runtime.getCurrentContext().nil;
Copy link
Member

Choose a reason for hiding this comment

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

its probably better to receive ThreadContext context instead or simply do a runtime.getNil() if the context is not needed anywhere else.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done. thanks for review!

Copy link
Member

Choose a reason for hiding this comment

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

@etehtsea @kares whether context is used or not we should always provide it as first argument to @JRubyMethod annotations so it will be future-proofed. We want to make sure these method signatures never need to change for native extension authors.

@etehtsea a second tidbit of info is getRuntime().getCurrentContext() is not super quick which lead to us adding the ability to pass ThreadContext to method bindings. We probably should have made it mandatory in 9k but we didn't :|

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@kares
Copy link
Member

kares commented Sep 2, 2016

should be 🍏 to go.

@headius
Copy link
Member

headius commented Sep 6, 2016

@etehtsea There seems to be a number of whitespace changes in your commits. Would it be possible to strip those out? Otherwise I'll need to merge manually and omit them myself.

@etehtsea
Copy link
Contributor Author

etehtsea commented Sep 7, 2016

@headius done

@kares kares modified the milestones: JRuby 9.1.6.0, JRuby 9.1.5.0 Sep 7, 2016
@kares kares merged commit 2aabd98 into jruby:master Sep 15, 2016
@etehtsea etehtsea deleted the gh-3954-signal-exception branch September 16, 2016 02:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants