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

bpo-39372: Clean header files of declared interfaces with no implementations #18037

Merged
merged 4 commits into from Jan 18, 2020

Conversation

@pablogsal
Copy link
Member

pablogsal commented Jan 17, 2020

@bedevere-bot

This comment has been minimized.

Copy link

bedevere-bot commented Jan 17, 2020

🤖 New build scheduled with the buildbot fleet by @pablogsal for commit d960460 🤖

If you want to schedule another build, you need to add the "🔨 test-with-buildbots" label again.

Copy link
Member

serhiy-storchaka left a comment

LGTM, except that the C API section is more appropriate for the NEWS entry.

@@ -22,8 +22,6 @@ typedef PyObject *(*PyCFunctionWithKeywords)(PyObject *, PyObject *,
typedef PyObject *(*_PyCFunctionFastWithKeywords) (PyObject *,
PyObject *const *, Py_ssize_t,
PyObject *);
typedef PyObject *(*PyNoArgsFunction)(PyObject *);

This comment has been minimized.

Copy link
@serhiy-storchaka

serhiy-storchaka Jan 17, 2020

Member

This is the definition. But it is not used, and never was used in CPython, and using it in extensions is likely an indication of an error.

@@ -81,8 +81,6 @@ PyAPI_FUNC(int) PyImport_ImportFrozenModule(
const char *name /* UTF-8 encoded string */
);

PyAPI_DATA(PyTypeObject) PyNullImporter_Type;

This comment has been minimized.

Copy link
@serhiy-storchaka

serhiy-storchaka Jan 17, 2020

Member

It was implemented in 3.2. Removing the implementation in 3.3 was technically the breakage of the stable ABI. But is too late to fix this.

Include/bytesobject.h Show resolved Hide resolved
@pablogsal pablogsal force-pushed the pablogsal:bpo-39372 branch from 83ffc89 to 75af3a6 Jan 17, 2020
@@ -0,0 +1,9 @@
Clean header files of interfaces defined but with no implementation. The
public API symbols being removed are:

This comment has been minimized.

Copy link
@serhiy-storchaka

serhiy-storchaka Jan 17, 2020

Member

I afraid that using multi-paragraph news entry can cause problems when files be merged into a single NEWS file.

This comment has been minimized.

Copy link
@pablogsal

pablogsal Jan 17, 2020

Author Member

Thanks for pointing that out! ... it was too good to be true (I couldn't check at the time I pushed this change).

@pablogsal pablogsal merged commit cd7db76 into python:master Jan 18, 2020
9 checks passed
9 checks passed
Docs
Details
Windows (x86)
Details
Windows (x64)
Details
macOS
Details
Ubuntu
Details
Azure Pipelines PR #20200117.52 succeeded
Details
bedevere/issue-number Issue number 39372 found
Details
bedevere/news News entry found in Misc/NEWS.d
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@pablogsal pablogsal deleted the pablogsal:bpo-39372 branch Jan 18, 2020
@pablogsal

This comment has been minimized.

Copy link
Member Author

pablogsal commented Jan 18, 2020

Thanks, Serhiy for the review!

@serhiy-storchaka

This comment has been minimized.

Copy link
Member

serhiy-storchaka commented Jan 18, 2020

Thank you for your great work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.