Creates PRuntimeException and audits eval package #1719
Conversation
Updates eval documentation
Removes PErrorListenerException and PErrorException
| } catch (t: Throwable) { | ||
| throw PErrors.internalErrorException(t) | ||
| } |
There was a problem hiding this comment.
I haven't checked this locally but shouldn't the non-PRuntimeExceptions be caught? I guess not a big deal to add an additional catch just in case.
There was a problem hiding this comment.
Wdym? We are catching Throwable here
There was a problem hiding this comment.
To rephrase, I was initially thinking that all the Throwable exceptions were already wrapped in a PRuntimeException, but seems like we need this additional catch in case there's some exception that we haven't already wrapped in a PRuntimeException. For example (this specific case has been fixed by #1720), 1.0 / 0.0 would give a "java.lang.ArithmeticException: / by zero" which would be wrapped in a PRuntimeException with code PError.INTERNAL_ERROR rather than a PRuntimeException with code PError.DIVISION_BY_ZERO. So the following two cases would give different error codes:
1 / 0 -- division by zero error
1.0 / 0.0 -- internal error
I think this catch(throwable) is fine for now, but we should make sure exceptions we throw try to use the existing PRuntimeException codes.
2 participants
Description
License Information
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.