Skip to content

Conversation

@ghost
Copy link

@ghost ghost commented Nov 13, 2019

This is a WIP pull request for PEP 573.

The purpose of this PEP is to provide a way to access per-module state from methods belonging to classes defined in extension modules.

https://bugs.python.org/issue38787

Copy link
Contributor

Choose a reason for hiding this comment

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

Py_XINCREF()?

Copy link
Contributor

Choose a reason for hiding this comment

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

What is the guarantee that prevents the two tuple item accesses (here and below) from running out of bounds?
Could that be worth a comment, at least?

Copy link
Member

Choose a reason for hiding this comment

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

A for loop might be better: first use PyTuple_Size to also check the type, then use the faster PyTuple_GET_ITEM.

Copy link
Member

@encukou encukou left a comment

Choose a reason for hiding this comment

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

Lots of comments on first glance:

Include/object.h Outdated
Comment on lines 180 to 184
Copy link
Member

Choose a reason for hiding this comment

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

This should be unnecessary.

Copy link
Member

Choose a reason for hiding this comment

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

Can we use the existing PyCFunction_Type for methods?
The METH_METHOD flag should signal that the underlying struct is PyCMethodObject; you'll also want to set tp_itemsize and the use PyObject_GC_NewVar.

Copy link
Member

Choose a reason for hiding this comment

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

This looks unnecessary; the PEP only mentions PyCFunction_GET_CLASS.

Include/object.h Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Oops, this is missing from the PEP's summary.

Include/object.h Outdated
Comment on lines 213 to 216
Copy link
Member

Choose a reason for hiding this comment

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

This is duplicates the definition in Include/internal/object.h.

Comment on lines 127 to 131
Copy link
Member

Choose a reason for hiding this comment

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

This should be much simpler, since PyCFunction_NewEx(...) is defined as PyCMethod_New(..., NULL).

Comment on lines 77 to 82
Copy link
Member

Choose a reason for hiding this comment

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

This looks like it would allow (METH_VARARGS | METH_METHOD) or METH_FASTCALL | METH_METHOD

Copy link
Member

Choose a reason for hiding this comment

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

A for loop might be better: first use PyTuple_Size to also check the type, then use the faster PyTuple_GET_ITEM.

Copy link
Member

Choose a reason for hiding this comment

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

Could you also test with another slot, not only __init__?

Copy link
Member

Choose a reason for hiding this comment

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

Is it always named cls?

@ghost ghost closed this Jan 22, 2020
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants