-
-
Notifications
You must be signed in to change notification settings - Fork 31.1k
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
bpo-33407: Implement Py_DEPRECATED() on Windows #8980
bpo-33407: Implement Py_DEPRECATED() on Windows #8980
Conversation
ac3ad87
to
64de78b
Compare
64de78b
to
72324ab
Compare
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.
LGTM.
@serhiy-storchaka, @zooba: Would you mind to double check?
This breaks compatibility. |
How? Would you mind to elaborate? |
The current usage: extern int foo(int) Py_DEPRECATED(3.2); This PR changes it to Py_DEPRECATED(3.2) extern int foo(int); It requires to rewrite all code that uses |
Ok, but we have to make the change to support Visual Studio. Do you think
that the macro is used outside Python itself?
|
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.
@ZackerySpytz: Can you please explain that Py_DEPRECATED() macro must now be used at the start of the function definition, rather than at the end, in a new "Changes in the C API" section at https://docs.python.org/dev/whatsnew/3.8.html#porting-to-python-3-8 ?
See https://docs.python.org/dev/whatsnew/3.7.html#changes-in-the-c-api for an example of the Python 3.7 documentation.
@ZackerySpytz: Can you please confirm that using Py_DEPRECATED() at the start of the function (rather than the end) already works in Python 3.7?
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
@vstinner I have added a What's New entry for this change.
Yes, I can confirm using Py_DEPRECATED() at the start of the function already worked before this change. |
Thanks for making the requested changes! @vstinner: please review the changes made to this pull request. |
@vstinner, I have implemented all of your requested changes, and Serhiy Storchaka has also now approved this PR. Please have a look. |
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 have requests on the documentation part.
Misc/NEWS.d/next/Windows/2018-08-28-17-23-49.bpo-33407.ARG0W_.rst
Outdated
Show resolved
Hide resolved
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
Two deprecation warnings are emitted on MSVC because of this PR; they can be seen on AppVeyor (e.g. https://ci.appveyor.com/project/python/cpython/builds/24580662#L125):
I had opened https://bugs.python.org/issue36935 and GH-13355 to fix these warnings. |
@vstinner I have made the requested changes; please review again. |
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.
LGTM.
https://bugs.python.org/issue33407