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

Complete removal of PyCXX #3723

Merged
merged 2 commits into from Oct 27, 2014
Merged

Conversation

ianthomas23
Copy link
Member

This PR completes the removal of PyCXX from matplotlib following on from PR #3646.

There are 2 commits. The first converts the _tri module, the last module to use PyCXX, to use the Python/C API instead. I've followed the lead of #3646 so that we have some code consistency in terms of separate wrapper files, code layout, use of the same exception-catching macros, etc.

The second commit removes the PyCXX source code, the relevant sections of the setup scripts, and a few other places where it was mentioned.

It is always nice to write a PR that is such a net reduction in code!

@tacaswell tacaswell added this to the v1.5.x milestone Oct 26, 2014
@jenshnielsen
Copy link
Member

It looks good to me. I did a quick test locally and everything seems to work fine. 👍 on merging this from me.

tacaswell added a commit that referenced this pull request Oct 27, 2014
MNT : Complete removal of PyCXX
@tacaswell tacaswell merged commit 04e006c into matplotlib:master Oct 27, 2014
segs[i] = Py::asObject((PyObject*)py_line);
if (PyList_SetItem(segs, i, (PyObject*)py_line)) {
PyErr_SetString(PyExc_RuntimeError,
"Unable to set contour segments");
Copy link
Member

Choose a reason for hiding this comment

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

I think you need to Py_DECREF(segs) here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, well spotted.

@mdboom
Copy link
Member

mdboom commented Oct 27, 2014

Thanks for doing this! I appreciate how much work this was. Sorry about commenting after this was merged, but it looks like this was up for less than 24 hours!

@tacaswell
Copy link
Member

Sorry, I applied the logic of the other pycxx branch and merged it on trusting the test suite.

@mdboom
Copy link
Member

mdboom commented Oct 27, 2014

@tacaswell: No problem. It's more important that this works than anything else, so merging early is the best for that.

@ianthomas23
Copy link
Member Author

@tacaswell: What should I do now that this PR has been merged? Should I just write a new PR for the changes and refer back to this one?

@tacaswell
Copy link
Member

Yes, that is probably the best way

On Mon, Oct 27, 2014 at 3:06 PM, Ian Thomas notifications@github.com
wrote:

@tacaswell https://github.com/tacaswell: What should I do now that this
PR has been merged? Should I just write a new PR for the changes and refer
back to this one?


Reply to this email directly or view it on GitHub
#3723 (comment)
.

Thomas Caswell
tcaswell@gmail.com

mdboom added a commit that referenced this pull request Nov 3, 2014
@ianthomas23 ianthomas23 deleted the final_decxx branch July 8, 2021 18:21
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

5 participants