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

Allow catching both C++ and Python exceptions. #2615

Merged
merged 3 commits into from Sep 22, 2018

Conversation

robertwb
Copy link
Contributor

No description provided.

Copy link
Contributor

@scoder scoder left a comment

Choose a reason for hiding this comment

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

Definitely looks like a good feature, but I wonder if this doesn't block us from allowing C++ exceptions together with explicit Python exception return values.

@@ -189,20 +189,31 @@ def infer_sequence_item_type(env, seq_node, index_node=None, seq_type=None):

def get_exception_handler(exception_value):
if exception_value is None:
return "__Pyx_CppExn2PyErr();"
return "__Pyx_CppExn2PyErr();", False
elif (exception_value.type == PyrexTypes.c_char_type
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this collide with an exception handler function that returns a char?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The declarator is completely parsed by this point, and the type is a function type.

@@ -189,20 +189,31 @@ def infer_sequence_item_type(env, seq_node, index_node=None, seq_type=None):

def get_exception_handler(exception_value):
if exception_value is None:
return "__Pyx_CppExn2PyErr();"
return "__Pyx_CppExn2PyErr();", False
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice to have some kind of indication in this function what the True/False return values mean. At least a comment or docstring, but some named constant could also make it clearer. Or a named tuple?

return '%s(); if (!PyErr_Occurred()) PyErr_SetString(PyExc_RuntimeError , "Error converting c++ exception.");' % exception_value.entry.cname, False

def maybe_check_py_error(code, check_py_exception, pos, nogil):
if check_py_exception:
Copy link
Contributor

Choose a reason for hiding this comment

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

Indentation. :)

@@ -737,13 +737,16 @@ def analyse(self, return_type, env, nonempty=0, directive_locals=None, visibilit
self.exception_value = self.exception_value.analyse_const_expression(env)
if self.exception_check == '+':
exc_val_type = self.exception_value.type
print exc_val_type
Copy link
Contributor

Choose a reason for hiding this comment

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

This breaks Py3 compatibility. ;-)

@@ -440,6 +440,13 @@ called, which allows one to do custom C++ to Python error "translations." If
raise_py_error does not actually raise an exception a RuntimeError will be
raised.

There is also the special form::

cdef int raise_py_or_cpp() except +*
Copy link
Contributor

Choose a reason for hiding this comment

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

This suggests that except + is rather orthogonal to the Python exception return value. But except +-1 looks funny … and it's very ambiguous.

Does @cython.exceptval('+') actually work in pure mode? There could be a way out in that corner …

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is orthogonal, and there's not really a good way to mix them. The exception value is a return value in the normal case, and a ~error handler in the + case. One might want a return value or an error handler or neither or both.

Right now, however, you can't safely use a function that may throw C++ or Python exceptions.

I do like the idea of using decorators, perhaps with keyword arguments rather than magic characters.

@scoder
Copy link
Contributor

scoder commented Sep 22, 2018

Looks good enough to me now. The travis failure is due to the code checker, one of the code generation lines is rather long.

Should we include this in 0.29?

@scoder
Copy link
Contributor

scoder commented Sep 22, 2018

Or do you want to try finding a better API with decorators first?

@robertwb
Copy link
Contributor Author

I'm going to go ahead and merge this and we can get it in 0.29.

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.

None yet

2 participants