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

Adds support for Py_LIMITED_API #14

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Adds support for Py_LIMITED_API #14

wants to merge 3 commits into from

Conversation

stt
Copy link

@stt stt commented May 25, 2018

Here's changes that were required to generate PEP384 compliant code (#13) for my use-case. It's likely missing various bits and pieces to be a complete implementation though.

Initially I considered adding an option to pybindgen that generates either stable API or traditional code, but in the end it made more sense to do it with preprocessor conditions. This does add a bit of length to the C output.

Didn't look into any new tests yet, tests/wscript ran successfully but not with -DPy_LIMITED_API for the moment.

@stt
Copy link
Author

stt commented May 25, 2018

There was one issue with Container._generate_container_constructor that lead to segfault with limited API for some reason. Not sure what that was about exactly (any ideas?) but since it seems to work fine with the second option of iterative copy, I just put it behind a condition.

@gjcarneiro
Copy link
Owner

This is a pretty cool feature to have. Thanks!

However I don't like one thing about this approach: too many ifdefs, too many code paths.

I am perfectly happy to make the "limited API" way of doing things the default (as long as it also works in Python 2.7). I am not so happy with the ifdefs. I would prefer to use ifdef in only in case it's not possible to support something with limited api and you have to disable that code in that case. For the cases where a feature can work with and without limited api, can you find a way that makes it work with both without ifdefs?

For example:

    #ifdef Py_LIMITED_API
    PyObject_DEL(self);
    #else
    Py_TYPE(self)->tp_free((PyObject*)self);
    #endif

Can this simply become PyObject_DEL(self); in both cases, without the ifdef?

@stt
Copy link
Author

stt commented May 26, 2018

Hi Gustavo, thanks for looking into it. I was thinking the same about the ifdefs and added PBG_SETATTR macro to get around those with tp_dict but hadn't used it yet.
The macro and PyObject_DEL seem to work with 2.7.12 and 3.5.2 at least (based on PYTHON=/usr/bin/python* ./waf configure check) so here's a commit for those changes.

Do you think it would be ok to merge the limited API feature in stages? I'm just doubting I'd have time/knowledge to test all of the language features for corner cases where bindings maybe generated that doesn't compile with Py_LIMITED_API.

@stt
Copy link
Author

stt commented May 26, 2018

Scratch that about PyObject_DEL, I had -Werror defined locally that masked the tests failing with "GC object already tracked". Not sure what the issue there is exactly but if no other ideas are forthcoming at least we could replace those ifdefs also with a macro?

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.

2 participants