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

Expand LIMITED_API support #3311

Merged
merged 14 commits into from Feb 18, 2020
Merged

Conversation

eduardo-elizondo
Copy link
Contributor

@eduardo-elizondo eduardo-elizondo commented Jan 14, 2020

Expands LIMITED_API support by adding fixes to previously unhandled cases. This now brings down the list of non-working tests in limited_api_bugs.txt down to 5. These are the fixes in this PR:

Heap types:
Made CyFunctionType and FusedFunctionType into heap types (through FetchCommonTypeFromSpec). Also, updated the type creation code to now support subclassing through PyType_FromSpecWithBases.

Cythonize multiple files:
The code generator was creating duplicate types when more than one file was cythonized. (i.e. there was an incorrect for module in modules:). This has moved the code to generate_c_class_declarations where it now generates the types only once.

Fixes to cinit:
Fixed some cases where CYTHON_COMPILING_IN_LIMITED_API was hiding the initialization code. To fix this, I reverted back to the original initialization code and updated the function to work with the appropriate type accessors (i.e: PyType_GetSlot).

Misc:

  • Updated Pyx_Globals and GetVtable to now work with a module instead of a module dict.
  • Changed a Py_LIMITED_API flag to CYTHON_COMPILING_IN_LIMITED_API
  • Disabled CYTHON_PROFILE (just like pypy and pyston)
  • Fixed an incorrect use of PyObject_GetItem to the correct PyObject_GetAttr
  • Added -Wno-unused-function to reduce noise in LIMITED_API tests.

See #2542

@eduardo-elizondo
Copy link
Contributor Author

All tests are passing now

Cython/Compiler/Nodes.py Outdated Show resolved Hide resolved
Cython/Compiler/Nodes.py Outdated Show resolved Hide resolved
Cython/Utility/Builtins.c Outdated Show resolved Hide resolved
Cython/Utility/CommonStructures.c Show resolved Hide resolved
Cython/Compiler/ModuleNode.py Outdated Show resolved Hide resolved
Cython/Utility/CythonFunction.c Outdated Show resolved Hide resolved
Cython/Utility/ImportExport.c Outdated Show resolved Hide resolved
@scoder
Copy link
Contributor

scoder commented Jan 16, 2020

-Wno-unused-function – this is unfortunate, since it covers actual problems. It might be ok as a temporary "can't deal with this annoyance now" kind of step, but it will eventually start to annoy users, so we should take care to avoid these warnings correctly. As annoying as it is, it's often easier to do a little cleanup along the way, rather than having to go back later and figure out why something isn't used. Just my opinion, I'm ok with this if it helps you get work done.

Cython/Compiler/Nodes.py Outdated Show resolved Hide resolved
Cython/Utility/CommonStructures.c Outdated Show resolved Hide resolved
Cython/Utility/CommonStructures.c Outdated Show resolved Hide resolved
Cython/Utility/CommonStructures.c Outdated Show resolved Hide resolved
Cython/Utility/CommonStructures.c Outdated Show resolved Hide resolved
Cython/Utility/CommonStructures.c Outdated Show resolved Hide resolved
@eduardo-elizondo
Copy link
Contributor Author

@scoder sorry for the delay, I was out for a couple of days! Anyways, I just fixed all the outstanding issues

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.

Thanks for the update, no pressure. Looks mostly good now, just a few comments.

Cython/Utility/CommonStructures.c Outdated Show resolved Hide resolved
Cython/Utility/CommonStructures.c Outdated Show resolved Hide resolved
Cython/Utility/CythonFunction.c Outdated Show resolved Hide resolved
@@ -2372,6 +2372,7 @@ def runtests(options, cmd_args, coverage=None):

if options.limited_api:
CFLAGS.append("-DCYTHON_LIMITED_API=1")
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I tried to point this out on the old PR, but: at some point I think this should add "-DPY_LIMITED_API" too, since at present you're only checking that this is internally consistent and not that it actually conforms to the limited API. I don't think it needs to come with this change though - I just think it should be on the TODO list

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Totally! Slowly working towards that direction. Starting with Py_LIMITED_API just made the whole thing explode. I imagine that with these last couple of changes, it should now be manageable to start tackling the remaining parts to get the PY_LIMITED_API flag in!

@eduardo-elizondo
Copy link
Contributor Author

eduardo-elizondo commented Feb 16, 2020

Addressed the last remaining comments. Next up after this change should be to start adding the Py_LIMITED_API flag itself!

@scoder scoder merged commit a87f498 into cython:master Feb 18, 2020
@scoder
Copy link
Contributor

scoder commented Feb 18, 2020

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