-
-
Notifications
You must be signed in to change notification settings - Fork 30.7k
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
gh-111489: Add PyList_FromArrayMoveRef() function #112975
Conversation
I tried to clear array items (or set bytes to a pattern, such as PYMEM_DEADBYTE= PyObject *const * args = &PyTuple_GET_ITEM(callargs, 0);
for (size_t i = 0; i < argcount; i++) {
Py_INCREF(args[i]);
}
PyObject *u = _PyTuple_FromArraySteal(args, argcount);
// ... check if u == NULL ...
// ... code using callargs ... So I chose to leave array unchanged. |
Add new functions to create tuple and list from arrays: * PyTuple_FromArray(). * PyTuple_FromArrayMoveRef(). * PyList_FromArrayMoveRef(). Add tests on new functions.
7b20ab0
to
b12186e
Compare
I would prefer to declare array type as |
I think it's confusing that the function consumes references when it succeeds but does not consume them when it fails. I would think that it should always consume references or never consume references. If it always consumes references, then one can write
I think the only-sometimes-consuming was why there used to be a lot of error-handling mistakes in module initialization code with
I would think that we would want to replicate the always-consuming "other functions" (e.g. PyTuple_SetItem) style, not the only-sometimes-consuming PyModule_AddObject style. Is there a reason to prefer this style? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Ref suffix is used in many new functions, would not it cause confusion?
What if use PyTuple_FromArrayMove()
or PyTuple_FromArrayMoveOwnership()
? Or maybe PyTuple_CopyFromArray()
and PyTuple_MoveFromArray()
?
Why there is no copying API for list?
tuple. | ||
|
||
* Return a new reference on success. | ||
* Set an exception and return NULL on error. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* Set an exception and return NULL on error. | |
* Set an exception and return ``NULL`` on error. |
# Test PyList_FromArrayMoveRef() | ||
list_fromarraymoveref = _testcapi.list_fromarraymoveref | ||
|
||
lst = [object() for _ in range(5)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would use the following example:
lst = [object() for _ in range(5)] | |
lst = [[i] for _ in range(5)] |
Better repr.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First, I used list("abdef")
, but all items are immortal and so tests didn't detect refcount bugs in my C code. I used object()
to make sure that each object is unique.
I don't think that repr is important here, the test is not supposed to fail :-)
copy = list_fromarraymoveref(lst) | ||
self.assertEqual(copy, lst) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test also with zero and negative size. with too large size (causing MemoryError or OverflowError), with array=NULL, and with NULLs in the array. We should know what cases raise exceptions, what cases crash and what cases are valid, and how errors are handled.
if (tuple == NULL) { | ||
for (Py_ssize_t i = 0; i < n; i++) { | ||
Py_DECREF(src[i]); | ||
for (Py_ssize_t i = 0; i < size; i++) { | ||
Py_DECREF(array[i]); | ||
} | ||
return NULL; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does not match the documentation.
I suggest discussing the name at: capi-workgroup/api-evolution#25 |
I only exposed what already existed. But yeah, when I wrote the PR, I also noticed that it seems like there is "a hole" in the API :-) I can add a "copy" API for list as well. |
I convert this PR to a draft until we agree on naming: capi-workgroup/api-evolution#25 |
I created this PR to have a more concrete API to discuss the naming. I prefer to close the PR for now, until we agree on naming. |
Add new functions to create tuple and list from arrays:
Add tests on new functions.
📚 Documentation preview 📚: https://cpython-previews--112975.org.readthedocs.build/