ruby-rice / rice

Ruby Interface for C++ Extensions
http://ruby-rice.github.io/
Other
378 stars 63 forks source link

Remove Custom Exception Handlers #177

Closed cfis closed 1 year ago

cfis commented 1 year ago

Working on adding std::map and I've gotten (re)annoyed and the big exception handling macros. What I've implemented is this pattern:

https://devblogs.microsoft.com/cppblog/exception-boundaries/

It possible of course to keep the idea of custom exception handlers around, but the default one simply throws. Which means the code looks like this:

try
  ....
except
   try
     throw
   catch
      .... tons of catch clauses
   end
end

I know there is a concept of chaining the handlers together also.

What I would like to do is remove the concept of custom exception handlers. I've yet to see any use case for them and they complicate the exception handling code. But maybe I'm missing something?

Thoughts?

jasonroelofs commented 1 year ago

Seeing it in action, I like the pattern. Definitely easier to understand than a macro.

ankane commented 1 year ago

fwiw, I'm using custom error handlers in Torch.rb to remove the C++ backtrace from error messages.

inline void handle_error(torch::Error const & ex) {
  throw Rice::Exception(rb_eRuntimeError, ex.what_without_backtrace());
}

I think they could also be nice for raising library-specific exceptions (like Torch::Error instead of RuntimeError), but maybe there's a better way to accomplish this.

cfis commented 1 year ago

Hmm, ok. Don't want to break your code.

Maybe a middle ground then. Something like:

So this is what is what I did so far:

https://github.com/jasonroelofs/rice/blob/master/rice/ruby_try_catch.hpp#L87

And this is how it looks:

https://github.com/jasonroelofs/rice/blob/master/rice/detail/NativeAttribute.ipp#L14

Note that the native attribute readers/writers never supported the exception handlers. Those are only used here:

https://github.com/jasonroelofs/rice/blob/master/rice/detail/NativeFunction.ipp#L254

So if we go down the route sketched above, then it would probably make sense to use the exception handler everywhere cpp_protect is used?

ankane commented 1 year ago

Thanks, don't think I have enough knowledge to comment on the implementation details above, but for my specific use case, I essentially want to wrap all torch::Error instances (rather than call add_handler for every class) if possible (if that helps at all with the API design).

cfis commented 1 year ago

I left the existing API, but replaced the macro with a lambda as discussed above.