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

Catch more exceptions from C++ standard containers #4079

Merged
merged 3 commits into from Apr 26, 2021
Merged

Catch more exceptions from C++ standard containers #4079

merged 3 commits into from Apr 26, 2021

Conversation

maxbachmann
Copy link
Contributor

No description provided.

Cython/Utility/CppConvert.pyx Outdated Show resolved Hide resolved
Cython/Utility/CppConvert.pyx Outdated Show resolved Hide resolved
Cython/Utility/CppConvert.pyx Outdated Show resolved Hide resolved
Cython/Utility/CppConvert.pyx Outdated Show resolved Hide resolved
@maxbachmann
Copy link
Contributor Author

Makes sense to take care of those changes in a separate PR 👍

@scoder
Copy link
Contributor

scoder commented Apr 3, 2021

Luckily, this broke a test in cpp_stl_conversion.pyx. It has a function that does this:

cdef string add_strings(string a, string b):
    return a + b

Looks innocent, but can now raise a Python exception. And the only way to propagate that exception is by marking this function as except *. That's fairly annoying, because it introduces a call to PyErr_Occurred() for every usage of this function.

I understand that this is correct, but it would be nice if we could mitigate this somehow. Not as part of this PR, though.

@maxbachmann
Copy link
Contributor Author

Assuming those are C++ strings shouldn't it be enough to mark the function as except +.

@scoder
Copy link
Contributor

scoder commented Apr 3, 2021

That's not how except + works. You can't use it on your own cdef functions. C++ exceptions cannot propagate through Cython code, they are always converted to Python exceptions directly.

I would also like to avoid repeated back-and-forth conversion between the two. It might seem ok for the trivial function above, but that's not the average real-world example. Once it's a Python exception, it should stay that until it's handled.

@da-woods
Copy link
Contributor

da-woods commented Apr 3, 2021

I understand that this is correct, but it would be nice if we could mitigate this somehow. Not as part of this PR, though.

It's hard to see how to mitigate that generally. One option would be to allow except + for really simple user-defined cdef functions that have no reference-counted types (including temporaries), no with-blocks (including with gil, but excluding most other Cython compiler directives), no try: ... except: blocks.

@maxbachmann
Copy link
Contributor Author

That's not how except + works

True forgot this. I did run into this and then placed the functions directly in C++ header files, so I do not have to run PyErr_Occurred each time I call the function.

@scoder scoder added this to the 3.0 milestone Apr 26, 2021
@scoder scoder merged commit 358418e into cython:master Apr 26, 2021
@scoder
Copy link
Contributor

scoder commented Apr 26, 2021

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants