Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Use pybind11 in ttconv module#25253
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thank you for opening your first PR into Matplotlib!
If you have not heard from us in a while, please feel free to ping@matplotlib/developers or anyone who has commented on the PR. Most of our reviewers are volunteers and sometimes things fall through the cracks.
You can also join uson gitter for real-time discussion.
For details on testing, writing docs, and our review process, please seethe developer guide
We strive to be a welcoming and open project. Please follow ourCode of Conduct.
src/_ttconv.cpp Outdated
| Py_DECREF(result); | ||
| PyObject* decoded = PyUnicode_DecodeLatin1(a, strlen(a), ""); | ||
| if (decoded == NULL) { | ||
| throw py::exception(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thank you very much for this! There are more suitable reviewers than me (@ianthomas23 and@anntzer comes to mind), but I think it would be good to replace this exception with "something else" (to get rid of thepy_exceptions.h import, which we want to get rid of as part of the PyBind11 conversion). Ithink thatstd::exception() is the way to go, possibly with some sensible error message. Although, of course, that general exception is not good etc, but at least it will be consistent with the current behavior.
(This exception is not tested in the original code either.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This should be py::error_already_set (https://pybind11.readthedocs.io/en/stable/advanced/exceptions.html#handling-exceptions-from-python-in-c).
Agreed this should be made to work without py_exceptions.h.
We should probably switch the whole thing to write to binary streams (and do something like fh.flush(); convert_ttf_to_ps(..., fh.buffer, ...); fh.buffer.flush() on the Python calling side), but that can be another PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I switched to usingerror_already_set. Note that this PR is keeping the original behavior onmain which catches this exception and ignores it:
Lines 139 to 142 inde8e9d0
| catch (const py::exception &) | |
| { | |
| returnNULL; | |
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
catches this exception and ignores it
No, returning NULL is the way for C-API functions to signal that an exception has been set and should be propagated to python-land...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Ah I see. I misinterpreted the original implementation. Thank you for the explanation!
a53da51 tof14a566Comparesrc/_ttconv.cpp Outdated
| pybind11::arg("filename"), | ||
| pybind11::arg("output"), | ||
| pybind11::arg("fonttype"), | ||
| pybind11::arg("glyph_ids"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
default value for glyph_ids is missing.
src/_ttconv.cpp Outdated
| const char *filename, | ||
| pybind11::object &output, | ||
| int fonttype, | ||
| pybind11::iterable glyph_ids) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Just make this argument astd::vector<int> and don't bother with doing the conversion yourself?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
With the default value ofNone, I think this needs to bepybind11::iterable*, to account for thenullptr. If we need to usestd::vector<int>, then we needstd::optional, which requiresC++17:
XREF: The note section from:https://pybind11.readthedocs.io/en/stable/advanced/functions.html#allow-prohibiting-none-arguments
Although I agree in principle, the question is if/when that will happen... If it is before 3.8 is released (i.e., in July(?)), the current PR may not be worthwhile, but considering that it has been around for 14 months it is not obvious that it will happen. Anyway, good that you bring it up, so your call@thomasjpfan . We appreciate your work, but there is a risk that it will be in vain in the sense that it will be removed before the next release. On the other hand, I still think it is worthwhile to merge this if finished as one do not know if it will be replaced and there are only details remaining. Sorry we didn't make it clear in#23846 that for this particular module there was another possible route. |
f14a566 tob074140Compare
I am okay with the changes made in this PR being removed because of#20866.
I agree that it makes sense to move forward with this PR anyways to get the benefits of using |
b074140 to8da08a7CompareThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I've left a few comments but I'd be happy for this to merged as it is.
Uh oh!
There was an error while loading.Please reload this page.
| catch (const py::exception &) | ||
| { | ||
| return NULL; | ||
| throw std::runtime_error(e.getMessage()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I am concerned that we don't exercise this line in the test suite as it would be good to confirm thatTTExceptions propagate throughpybind11 as expected. But given that we've evidently never had such a test it doesn't have to be done here, it can be punted to "future work".
| } | ||
| catch (...) | ||
| { | ||
| PyErr_SetString(PyExc_RuntimeError, "Unknown C++ exception"); | ||
| return NULL; | ||
| throw std::runtime_error("Unknown C++ exception"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This catch all isn't really needed aspybind11 catches C++ exceptions and converts them to Python ones. But I like the approach that support forpybind11 should happen with minimal changes to C++ code so it is fine to leave it as it is.
8da08a7 todcb5e31Comparedcb5e31 toc38348bCompareFrom a quick look it seems fine. Let's just get this in modulo CI. |
Thanks@thomasjpfan ! |
PR Summary
Towards#23846
This PR migrates the ttconv module to using pybind11. Most of the conversion uses
pybind11data structures. Some notes for reviewers:PyObject *that does not perform any reference counting.