bpo-33738: Fix macros which contradict PEP 384 by ctismer · Pull Request #7477 · python/cpython

ctismer · 2018-06-07T10:27:26Z

During development of the limited API support for PySide,
we saw an error in a macro that accessed a type field.

This patch fixes the 7 errors in the Python headers.
Macros which were not written as capitals were implemented
as function.

To do the necessary analysis again, a script was included that
parses all headers and looks for "->tp_" in sections which can
be reached with active limited API.

It is easily possible to call this script as a test.

Error listing:

../../Include/objimpl.h:243
#define PyObject_IS_GC(o) (PyType_IS_GC(Py_TYPE(o)) &&
(Py_TYPE(o)->tp_is_gc == NULL || Py_TYPE(o)->tp_is_gc(o)))
Action: commented only

../../Include/objimpl.h:362
#define PyType_SUPPORTS_WEAKREFS(t) ((t)->tp_weaklistoffset > 0)
Action: commented only

../../Include/objimpl.h:364
#define PyObject_GET_WEAKREFS_LISTPTR(o)
((PyObject **) (((char *) (o)) + Py_TYPE(o)->tp_weaklistoffset))
Action: commented only

../../Include/pyerrors.h:143
#define PyExceptionClass_Name(x)
((char )(((PyTypeObject)(x))->tp_name))
Action: implemented function

../../Include/abstract.h:593
#define PyIter_Check(obj)
((obj)->ob_type->tp_iternext != NULL &&
(obj)->ob_type->tp_iternext != &_PyObject_NextNotImplemented)
Action: implemented function

../../Include/abstract.h:713
#define PyIndex_Check(obj)
((obj)->ob_type->tp_as_number != NULL &&
(obj)->ob_type->tp_as_number->nb_index != NULL)
Action: implemented function

../../Include/abstract.h:924
#define PySequence_ITEM(o, i)
( Py_TYPE(o)->tp_as_sequence->sq_item(o, i) )
Action: commented only

https://bugs.python.org/issue33738

During development of the limited API support for PySide, we saw an error in a macro that accessed a type field. This patch fixes the 7 errors in the Python headers. Macros which were not written as capitals were implemented as function. To do the necessary analysis again, a script was included that parses all headers and looks for "->tp_" in serctions which can be reached with active limited API. It is easily possible to call this script as a test. Error listing: ../../Include/objimpl.h:243 #define PyObject_IS_GC(o) (PyType_IS_GC(Py_TYPE(o)) && \ (Py_TYPE(o)->tp_is_gc == NULL || Py_TYPE(o)->tp_is_gc(o))) Action: commented only ../../Include/objimpl.h:362 #define PyType_SUPPORTS_WEAKREFS(t) ((t)->tp_weaklistoffset > 0) Action: commented only ../../Include/objimpl.h:364 #define PyObject_GET_WEAKREFS_LISTPTR(o) \ ((PyObject **) (((char *) (o)) + Py_TYPE(o)->tp_weaklistoffset)) Action: commented only ../../Include/pyerrors.h:143 #define PyExceptionClass_Name(x) \ ((char *)(((PyTypeObject*)(x))->tp_name)) Action: implemented function ../../Include/abstract.h:593 #define PyIter_Check(obj) \ ((obj)->ob_type->tp_iternext != NULL && \ (obj)->ob_type->tp_iternext != &_PyObject_NextNotImplemented) Action: implemented function ../../Include/abstract.h:713 #define PyIndex_Check(obj) \ ((obj)->ob_type->tp_as_number != NULL && \ (obj)->ob_type->tp_as_number->nb_index != NULL) Action: implemented function ../../Include/abstract.h:924 #define PySequence_ITEM(o, i)\ ( Py_TYPE(o)->tp_as_sequence->sq_item(o, i) ) Action: commented only

During development of the limited API support for PySide, we saw an error in a macro that accessed a type field. This patch fixes the 7 errors in the Python headers. Macros which were not written as capitals were implemented as function. To do the necessary analysis again, a script was included that parses all headers and looks for "->tp_" in sections which can be reached with active limited API. It is easily possible to call this script as a test. Error listing: ../../Include/objimpl.h:243 (Py_TYPE(o)->tp_is_gc == NULL || Py_TYPE(o)->tp_is_gc(o))) Action: commented only ../../Include/objimpl.h:362 Action: commented only ../../Include/objimpl.h:364 ((PyObject **) (((char *) (o)) + Py_TYPE(o)->tp_weaklistoffset)) Action: commented only ../../Include/pyerrors.h:143 ((char *)(((PyTypeObject*)(x))->tp_name)) Action: implemented function ../../Include/abstract.h:593 ((obj)->ob_type->tp_iternext != NULL && \ (obj)->ob_type->tp_iternext != &_PyObject_NextNotImplemented) Action: implemented function ../../Include/abstract.h:713 ((obj)->ob_type->tp_as_number != NULL && \ (obj)->ob_type->tp_as_number->nb_index != NULL) Action: implemented function ../../Include/abstract.h:924 ( Py_TYPE(o)->tp_as_sequence->sq_item(o, i) ) Action: commented only

ctismer · 2018-06-07T18:15:36Z

Can I squash these commits, or do I need to re-send the whole thing? (Or leave it in pieces)

zware · 2018-06-07T18:17:52Z

They'll be squashed at merge time, don't worry about it until then :)

ned-deily · 2018-06-09T18:31:33Z

LGTM, although this is not an area of my expertise. Since we are getting very close to 3.7.0rc1 and it would be good to have this resolved, I'm going to merge this to master now so it gets some buildbot exposure. If anyone has any review comments, get them in real soon! Otherwise, I will backport and merge to 3.7 within the next 24 hours.

ned-deily · 2018-06-09T19:33:47Z

Looks like the change introduces a compile warning. Should that be addressed?:

..\Objects\exceptions.c(349): warning C4090: 'return': different 'const' qualifiers [D:\buildarea\3.x.ware-win81-release\build\PCbuild\pythoncore.vcxproj]

http://buildbot.python.org/all/#/builders/12/builds/960

ctismer · 2018-06-09T22:08:21Z

Yes it should be fixed. I was not aware that Python has const-ness, already.
I have amended my patch with "const char *". But since it is merged, I don't see the change.

ned-deily · 2018-06-09T22:16:36Z

@ctismer Thanks. The best thing would be to open a new PR but, if you can paste the diff here, I'll take care of it.

serhiy-storchaka · 2018-06-08T04:42:21Z

    ((obj)->ob_type->tp_iternext != NULL && \
     (obj)->ob_type->tp_iternext != &_PyObject_NextNotImplemented)
+#else
+PyAPI_FUNC(int) PyIter_Check(PyObject*);


Nitpick: it would look better if add a space between PyObject and *.

serhiy-storchaka · 2018-06-08T05:03:39Z

 #define PyExceptionClass_Name(x) \
     ((char *)(((PyTypeObject*)(x))->tp_name))
+#else
+     PyAPI_FUNC(char *) PyExceptionClass_Name(PyObject*);


Should return const char *. And please add a space between PyObject and *.

Will do.
My style is a bit spoiled by Qt's C++ style. They write "func(PyObject *self)"
but "func(PyObject*)".
I am preparing a diff as Ned said, but I can also repeat the PR.

serhiy-storchaka · 2018-06-08T05:07:28Z


+#ifndef Py_LIMITED_API
 #define PyExceptionClass_Name(x) \
     ((char *)(((PyTypeObject*)(x))->tp_name))


Actually (char *) is not needed. But it should be documented in What's New that PyExceptionClass_Name() now returns const char * instead of char * (there were similar breaking changes in 3.7).

serhiy-storchaka · 2018-06-08T05:19:02Z

 }

+#undef PyExceptionClass_Name
+char *


Return const char *.

serhiy-storchaka · 2018-06-08T05:20:23Z

 }

+#undef PyIter_Check
+int PyIter_Check(PyObject *obj)


Nitpick: Break a line between int and PyIter_Check.

Ok, will do. Although all other #undef's are without newline.

Eeek, misunderstood!
Sure, you wanted a line after int!
I must have been very tired...

serhiy-storchaka · 2018-06-10T04:58:04Z

Seems I forgot to press the "Submit review" button after adding comments.

ctismer · 2018-06-10T08:16:44Z

Ok, I hope I got all changes right. Doing a new PR.

serhiy-storchaka · 2018-06-10T11:57:33Z

Since this PR has been merged, my comments except the one about the result of PyExceptionClass_Name are outdated. They where just style nitpicks. And I have opened a separate issue for the latter.

tekknolagi · 2020-06-19T17:59:48Z

Was this ever backported to Python 3.7?

ned-deily · 2020-06-22T08:25:49Z

@tekknolagi No, I don't think they were ever backported to 3.7. See the discussion on the still open bug issue (https://bugs.python.org/issue33738) which is also the place for comments. In any case, with the upcoming release of 3.7.8, 3.7 has moved to security-fix mode, precluding changes like this.

the-knights-who-say-ni added the CLA signed label Jun 7, 2018

bedevere-bot added the awaiting review label Jun 7, 2018

ctismer requested a review from a team as a code owner June 7, 2018 17:17

ned-deily requested a review from serhiy-storchaka June 8, 2018 04:33

ned-deily merged commit ea62ce7 into python:master Jun 9, 2018

bedevere-bot removed the awaiting review label Jun 9, 2018

serhiy-storchaka reviewed Jun 10, 2018

View reviewed changes

ned-deily pushed a commit that referenced this pull request Jun 10, 2018

bpo-33738: Address review comments in GH #7477 (GH-7585)

8398713

serhiy-storchaka mentioned this pull request Jan 29, 2025

Remove broken PySequence_Fast() function from the limited C API capi-workgroup/decisions#55

Closed

6 tasks

Uh oh!

Conversation

ctismer commented Jun 7, 2018 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ctismer commented Jun 7, 2018

Uh oh!

zware commented Jun 7, 2018

Uh oh!

ned-deily commented Jun 9, 2018

Uh oh!

ned-deily commented Jun 9, 2018

Uh oh!

ctismer commented Jun 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ned-deily commented Jun 9, 2018

Uh oh!

serhiy-storchaka Jun 8, 2018

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka Jun 8, 2018

Choose a reason for hiding this comment

Uh oh!

ctismer Jun 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka Jun 8, 2018

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka Jun 8, 2018

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka Jun 8, 2018

Choose a reason for hiding this comment

Uh oh!

ctismer Jun 10, 2018

Choose a reason for hiding this comment

Uh oh!

ctismer Jun 23, 2018

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka commented Jun 10, 2018

Uh oh!

ctismer commented Jun 10, 2018

Uh oh!

serhiy-storchaka commented Jun 10, 2018

Uh oh!

tekknolagi commented Jun 19, 2020

Uh oh!

ned-deily commented Jun 22, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

ctismer commented Jun 7, 2018 •

edited by bedevere-bot

Loading

ctismer commented Jun 9, 2018 •

edited

Loading

ctismer Jun 10, 2018 •

edited

Loading