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

@kares kares Sep 1, 2016

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.

Loading

Copy link
Contributor Author

@etehtsea etehtsea Sep 1, 2016

Choose a reason for hiding this comment

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

done. thanks for review!

Loading

Copy link
Member

@enebo enebo Sep 1, 2016

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 :|

Loading

Copy link
Contributor Author

@etehtsea etehtsea Sep 1, 2016

Choose a reason for hiding this comment

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

done

Loading

@etehtsea etehtsea force-pushed the gh-3954-signal-exception branch from d604b64 to 572f7a7 Sep 1, 2016
@etehtsea etehtsea force-pushed the gh-3954-signal-exception branch from 572f7a7 to 3e4381d Sep 1, 2016
@kares kares added this to the JRuby 9.1.5.0 milestone Sep 2, 2016
@kares
Copy link
Member

@kares kares commented Sep 2, 2016

should be 🍏 to go.

Loading

@headius
Copy link
Member

@headius 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.

Loading

@etehtsea etehtsea force-pushed the gh-3954-signal-exception branch from 3e4381d to db2490b Sep 7, 2016
@etehtsea
Copy link
Contributor Author

@etehtsea etehtsea commented Sep 7, 2016

@headius done

Loading

@etehtsea etehtsea force-pushed the gh-3954-signal-exception branch from db2490b to 6b793be Sep 7, 2016
@kares kares added this to the JRuby 9.1.6.0 milestone Sep 7, 2016
@kares kares removed this from the JRuby 9.1.5.0 milestone Sep 7, 2016
@kares kares merged commit 2aabd98 into jruby:master Sep 15, 2016
1 of 2 checks passed
Loading
@etehtsea etehtsea deleted the gh-3954-signal-exception branch Sep 16, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants