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

gh-111489: Add PyList_FromArrayMoveRef() function #112975

Closed
wants to merge 1 commit into from

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented Dec 11, 2023

Add new functions to create tuple and list from arrays:

  • PyTuple_FromArray().
  • PyTuple_FromArrayMoveRef().
  • PyList_FromArrayMoveRef().

Add tests on new functions.


📚 Documentation preview 📚: https://cpython-previews--112975.org.readthedocs.build/

@vstinner
Copy link
Member Author

I tried to clear array items (or set bytes to a pattern, such as PYMEM_DEADBYTE=0xDD) in PyTuple_FromArrayMoveRef() and PyList_FromArrayMoveRef() in debug mode, but it broke current code which expect the array to remain usable. Pseudo-code:

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.
@vstinner
Copy link
Member Author

PyObject* PyList_FromArrayMoveRef(PyObject *const *array, Py_ssize_t size)

I would prefer to declare array type as PyObject **, but since the function does not modify the array, I chose to declare the type using const.

@sweeneyde
Copy link
Member

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

    PyObject *pair[2];
    pair[0] = pair[1] = something;
    return _PyList_FromArraySteal(pair);

I think the only-sometimes-consuming was why there used to be a lot of error-handling mistakes in module initialization code with PyModule_AddObject. The docs note that

Unlike other functions that steal references, PyModule_AddObject() only releases the reference to value on success.

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?

Copy link
Member

@serhiy-storchaka serhiy-storchaka left a 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.
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
* 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)]
Copy link
Member

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:

Suggested change
lst = [object() for _ in range(5)]
lst = [[i] for _ in range(5)]

Better repr.

Copy link
Member Author

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 :-)

Comment on lines +351 to +352
copy = list_fromarraymoveref(lst)
self.assertEqual(copy, lst)
Copy link
Member

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.

Comment on lines 401 to 406
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;
}
Copy link
Member

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.

@vstinner
Copy link
Member Author

What if use PyTuple_FromArrayMove() or PyTuple_FromArrayMoveOwnership()? Or maybe PyTuple_CopyFromArray() and PyTuple_MoveFromArray()?

I suggest discussing the name at: capi-workgroup/api-evolution#25

@vstinner
Copy link
Member Author

Why there is no copying API for list?

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.

@vstinner vstinner marked this pull request as draft December 14, 2023 23:59
@vstinner
Copy link
Member Author

I convert this PR to a draft until we agree on naming: capi-workgroup/api-evolution#25

@vstinner
Copy link
Member Author

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.

@vstinner vstinner closed this Dec 20, 2023
@vstinner vstinner deleted the pylist_fromarray branch December 20, 2023 11:04
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.

3 participants