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

ffi cleanup: pystrtod to pythonrun #1830

Merged
merged 5 commits into from Aug 28, 2021
Merged

Conversation

deantvv
Copy link
Contributor

@deantvv deantvv commented Aug 25, 2021

For #1289

Move multiple limited api to cpython. Many API in pythonrun are
removed in python 3.10. For a reminder, I add comment "will be removed
in 3.10" for that.

There is also some function and macro share the same name (documented
below) in cpython, which I choose to skip macro definition.

  • PyRun_String
  • PyRun_AnyFile
  • PyRun_AnyFileEx
  • PyRun_AnyFileFlags

@davidhewitt
Copy link
Member

davidhewitt commented Aug 25, 2021

Thanks, looks like a great tidy up!

For all APIs removed in Python 3.10 can you use #[cfg(not(Py_3_10))] instead of a comment please?

@deantvv deantvv force-pushed the ffi-cleanup-0825 branch 2 times, most recently from ba8b071 to 530b086 Compare August 26, 2021 10:24
@deantvv
Copy link
Contributor Author

deantvv commented Aug 26, 2021

Thanks, looks like a great tidy up!

For all APIs removed in Python 3.10 can you use #[cfg(not(Py_3_10))] instead of a comment please?

Done and also update changelog.

@deantvv deantvv force-pushed the ffi-cleanup-0825 branch 2 times, most recently from e98d73b to 1819733 Compare August 26, 2021 15:55
Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

Thanks very much, this looks great!

Just a few final tweaks needed and then we're ready to merge...

CHANGELOG.md Outdated Show resolved Hide resolved
src/ffi/pythonrun.rs Outdated Show resolved Hide resolved
src/ffi/pythonrun.rs Outdated Show resolved Hide resolved
src/ffi/pythonrun.rs Outdated Show resolved Hide resolved
@deantvv deantvv force-pushed the ffi-cleanup-0825 branch 2 times, most recently from f5e6fa5 to c871390 Compare August 27, 2021 15:06
Move multiple limited api to cpython. Many API in `pythonrun` are
removed in python 3.10.

There is also some function and macro share the same name (documented
below) in cpython, which I choose to skip macro definition.
- PyRun_String
- PyRun_AnyFile
- PyRun_AnyFileEx
- PyRun_AnyFileFlags
Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

Looks great, thanks very much! I've just added some suggestions to also configure the last remaining symbols in this file for PyPy. I'll add that commit and then merge!

src/ffi/cpython/pythonrun.rs Show resolved Hide resolved
src/ffi/cpython/pythonrun.rs Show resolved Hide resolved
src/ffi/cpython/pythonrun.rs Show resolved Hide resolved
src/ffi/cpython/pythonrun.rs Show resolved Hide resolved
src/ffi/cpython/pythonrun.rs Show resolved Hide resolved
src/ffi/cpython/pythonrun.rs Outdated Show resolved Hide resolved
src/ffi/cpython/pythonrun.rs Show resolved Hide resolved
src/ffi/cpython/pythonrun.rs Show resolved Hide resolved
src/ffi/pythonrun.rs Outdated Show resolved Hide resolved
src/ffi/pythonrun.rs Outdated Show resolved Hide resolved
@davidhewitt davidhewitt merged commit cf0e8a6 into PyO3:main Aug 28, 2021
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

2 participants