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-42100: Add _PyType_GetModuleByDef #22835

Merged
merged 1 commit into from
Nov 3, 2020
Merged

Conversation

encukou
Copy link
Member

@encukou encukou commented Oct 20, 2020

@encukou
Copy link
Member Author

encukou commented Oct 20, 2020

See #22838 for example usage.

Copy link
Member

@corona10 corona10 left a comment

Choose a reason for hiding this comment

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

nit comment.

IMHO, we can use C99 features since some of the module already use other c99 features.
(for example, struct init)

assert(PyType_Check(type));
assert(type->tp_mro);
int i;
for (i = 0; i < PyTuple_GET_SIZE(type->tp_mro); i++) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
for (i = 0; i < PyTuple_GET_SIZE(type->tp_mro); i++) {
for (int i = 0; i < PyTuple_GET_SIZE(type->tp_mro); i++) {

{
assert(PyType_Check(type));
assert(type->tp_mro);
int i;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
int i;

@encukou
Copy link
Member Author

encukou commented Oct 26, 2020

IMO that's a code style issue, and PEP7 has nothing against it. I'd rather keep it as is.
The declaration is already C99-style: it's not at the beginning of the function :)

Copy link
Member

@shihai1991 shihai1991 left a comment

Choose a reason for hiding this comment

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

Thanks, petr. I have no any other comment in here.
PS: I have reviewed it months ago in fork repo~

@encukou encukou merged commit 57aaaa8 into python:master Nov 3, 2020
@encukou encukou deleted the pep-573-walker branch November 3, 2020 21:27
@encukou
Copy link
Member Author

encukou commented Nov 3, 2020

Thanks!

And thanks for the other review – this PR includes the changes you pointed out :)

@vstinner
Copy link
Member

vstinner commented Nov 4, 2020

When a type has no subclass but a long MRO, _PyType_GetModuleByDef() has to iterate on the N parent classes before reaching the last one which will match. Would it be more efficient to iterator on the MRO in the reverse order?

It would make the function slower for subclasses, but faster for direct instance of the type.

@vstinner
Copy link
Member

vstinner commented Nov 4, 2020

Usually, when I read "get", I expect a O(1) operation. I'm fine if the first call fills a cache and is slower. But here, every call has a complexity of O(n) where n is the length of the MRO tuple. Would it make sense to rename the function to _PyType_FindModuleByDef() to announce that it can be slow?

Anyway, thanks for adding this function! It will unblock porting many extension modules to multi-phase init and heap types in https://bugs.python.org/issue1635741 !

@vstinner
Copy link
Member

vstinner commented Nov 4, 2020

About _PyType_FindModuleByDef() name, here is a more concrete example: the PR #23124 converts the array extension to multi-phase init. The current PR defines:

#define get_array_state_by_type(tp) \
    (get_array_state(_PyType_GetModuleByDef(tp, &arraymodule)))
#define get_array_state_by_class(cls) \
    (get_array_state(PyType_GetModule(cls)))

I would prefer to use "find" in the first macro, to better highlight that "get_array_state_by_type()" is a fast attribute access:

#define find_array_state_by_type(tp) \
    (get_array_state(_PyType_FindModuleByDef(tp, &arraymodule)))
#define get_array_state_by_type(cls) \
    (get_array_state(PyType_GetModule(cls)))

@encukou
Copy link
Member Author

encukou commented Nov 4, 2020

Usually, when I read "get", I expect a O(1) operation.

Hm, I don't expect that. "get" can mean many things :)

I'll keep it in mind for if/when it becomes public API. I don't think it's worth changing the name now, after all the discussion.

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.

6 participants