Skip to content

Conversation

@pablogsal
Copy link
Member

@pablogsal pablogsal commented Jul 5, 2020

Copy link
Member

@brandtbucher brandtbucher left a comment

Choose a reason for hiding this comment

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

I like the tests! Noticed one small opportunity to simplify the register call, but looks good either way:

Py_DECREF((PyObject *)&Arraytype);
return -1;
}
PyObject *res = PyObject_CallMethod(mutablesequence, "register", "O", (PyObject *)&Arraytype);
Copy link
Member

@brandtbucher brandtbucher Jul 5, 2020

Choose a reason for hiding this comment

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

Suggested change
PyObject *res = PyObject_CallMethod(mutablesequence, "register", "O", (PyObject *)&Arraytype);
_Py_IDENTIFIER(register);
PyObject *res = _PyObject_CallMethodIdOneArg(mutablesequence, &PyId_register,
(PyObject *)&Arraytype);

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, @brandtbucher. The reason I originally did not add _Py_IDENTIFIER is so we don't add more to the global/static state so it does not make future sub interpreters isolation harder, especially given that this code will run only at module import time.

@pablogsal pablogsal merged commit e51dd9d into python:master Jul 5, 2020
@pablogsal
Copy link
Member Author

Thanks for the review!

@bedevere-bot

This comment has been minimized.

@pablogsal
Copy link
Member Author

Hi! The buildbot aarch64 RHEL7 3.x has failed when building commit e51dd9d.

The failure is unrelated to this PR:

1 test altered the execution environment:
test_asyncio

shihai1991 pushed a commit to shihai1991/cpython that referenced this pull request Jul 6, 2020
@pablogsal pablogsal deleted the bpo-29727 branch May 19, 2021 18:59
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