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

eval functions with panic safe implementation #79

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

danielpclark
Copy link
Contributor

@danielpclark danielpclark commented Feb 1, 2018

This requires a version bump in ruby-sys for the new eval methods and then the Cargo.toml here can be set to that version.

The code works as is now but I have a few questions. Before those questions I'll explain that I've chosen to use rb_eval_string_protect for VM::eval since it allows us to get a standard Result<AnyObject, c_int> back and we won't need to worry about it crashing our program. The rb_eval_string I added as an unsafe VM::eval_str because any exceptions raised crash the program. So until we have exceptions implemented I figured this would be the safer Rust way to do it.

My questions are whether we should have VM::eval_str at all since I've marked it as unsafe. Should the VM::eval be enough and I remove the other method since we don't have exception handling? If so I would suppose the binding for that should go for now, or maybe #[allow(dead_code)]?

Of course once we have exceptions implemented then I would expect the VM::eval method to switch from the rb_eval_string_protect to rb_eval_string.

Thinking out loud: But thinking about the way Rust works I believe that any time an exception is, or can be, raised then methods should return a Result<AnyObject, Error> and it would be nice for Result to implement the try_convert_to directly on it. Errors can still be of type AnyObject but return types in general would make more sense as a Result and just re-implement casting to the Result types Ok and Err. Those are my thoughts on that.

TODO

  • Wait for ruby-sys update and update Cargo.toml

@danielpclark
Copy link
Contributor Author

After looking into Ruby's Exception handling code I believe rb_eval_string_protect is the right choice. With that if it comes back as an Err() we can then use the rb_errinfo() -> Value to retrieve the instance of the Exception class.

@danielpclark
Copy link
Contributor Author

danielpclark commented Feb 16, 2018

Thinking more about this I want to update this feature after steveklabnik/ruby-sys#28 is merged. I want to update eval's error path to return an AnyObject of the exception class raised.

@danielpclark
Copy link
Contributor Author

danielpclark commented Feb 16, 2018

Alright the Rust eval method for the Ruby API will return a Result<AnyObject, AnyObject> allowing us to work with exceptions perfectly between Ruby and Rust. An example is provided.

@danielpclark danielpclark changed the title eval functions (needs feedback) eval functions (needs ruby-sys PR merge) Feb 16, 2018
@danielpclark danielpclark changed the title eval functions (needs ruby-sys PR merge) eval functions with panic safe implementation Mar 6, 2018
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.

1 participant