-
-
Notifications
You must be signed in to change notification settings - Fork 925
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
Conversation
public IRubyObject initialize(IRubyObject[] args, Block block) { | ||
final Ruby runtime = getRuntime(); | ||
int argnum = 1; | ||
IRubyObject sig = runtime.getCurrentContext().nil; |
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.
its probably better to receive ThreadContext context
instead or simply do a runtime.getNil()
if the context is not needed anywhere else.
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.
done. thanks for review!
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.
@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 :|
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.
done
d604b64
to
572f7a7
Compare
572f7a7
to
3e4381d
Compare
should be 🍏 to go. |
@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. |
3e4381d
to
db2490b
Compare
@headius done |
db2490b
to
6b793be
Compare
Fixes #3954