Skip to content

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

Merged
pablogsal merged 4 commits intopython:masterfrom
pablogsal:bpo-39372
Jan 18, 2020
Merged

bpo-39372: Clean header files of declared interfaces with no implementations#18037
pablogsal merged 4 commits intopython:masterfrom
pablogsal:bpo-39372

Conversation

@pablogsal
Copy link
Copy Markdown
Member

@pablogsal pablogsal commented Jan 17, 2020

@pablogsal pablogsal requested a review from 1st1 as a code owner January 17, 2020 17:48
@pablogsal pablogsal added 🔨 test-with-buildbots Test PR w/ buildbots; report in status section and removed awaiting core review labels Jan 17, 2020
@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jan 17, 2020
@pablogsal pablogsal added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jan 17, 2020
@bedevere-bot
Copy link
Copy Markdown

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

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

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jan 17, 2020
@python python deleted a comment from bedevere-bot Jan 17, 2020
Copy link
Copy Markdown
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

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

Comment thread Include/methodobject.h
typedef PyObject *(*_PyCFunctionFastWithKeywords) (PyObject *,
PyObject *const *, Py_ssize_t,
PyObject *);
typedef PyObject *(*PyNoArgsFunction)(PyObject *);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

Comment thread Include/import.h
const char *name /* UTF-8 encoded string */
);

PyAPI_DATA(PyTypeObject) PyNullImporter_Type;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

Comment thread Include/bytesobject.h
@@ -0,0 +1,9 @@
Clean header files of interfaces defined but with no implementation. The
public API symbols being removed are:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member Author

@pablogsal pablogsal Jan 17, 2020

Choose a reason for hiding this comment

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

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
@pablogsal pablogsal deleted the bpo-39372 branch January 18, 2020 03:15
@pablogsal
Copy link
Copy Markdown
Member Author

Thanks, Serhiy for the review!

@serhiy-storchaka
Copy link
Copy Markdown
Member

Thank you for your great work!

shihai1991 pushed a commit to shihai1991/cpython that referenced this pull request Jan 31, 2020
…tations (pythonGH-18037)

The public API symbols being removed are:

_PyBytes_InsertThousandsGroupingLocale, _PyBytes_InsertThousandsGrouping, _Py_InitializeFromArgs, _Py_InitializeFromWideArgs, _PyFloat_Repr, _PyFloat_Digits,
_PyFloat_DigitsInit, PyFrame_ExtendStack, _PyAIterWrapper_Type, PyNullImporter_Type, PyCmpWrapper_Type, PySortWrapper_Type, PyNoArgsFunction.
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.

4 participants