Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork32.4k
DOC: emphasize the need to always call PySequence_Fast#11140
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Is not this a PyPy issue that |
The functions work for |
@serhiy-storchaka: I think the point is that requiring those functions to work on lists and tuples puts nasty lifetime constraints on pypy, which could be lifted simply by relaxing the documentation to saying" |
I think this change needs more wide discussion. Please open an issue on thebug tracker and ask the question on thePython-Dev mailing list. |
@mattip Hi, have you been able to open an issue on bugs.python.org about it as requested by Serhiy? |
@serhiy-storchaka what do you see as controversial? Perhaps you could suggest a phrasing that would not require such an extensive discussion for a documentation clarification? |
Doc/c-api/sequence.rst Outdated
| .. c:function:: PyObject* PySequence_Fast_GET_ITEM(PyObject *o, Py_ssize_t i) | ||
| Return the *i*\ th element of *o*, assuming that *o* was returned by | ||
| :c:func:`PySequence_Fast`, *o* is not *NULL*, and that *i* is within bounds. | ||
| Equivalent to :c:func:`PySequence_GetItem` but faster. |
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.
Not exactly. At the very least, it has a difference analogous to the one betweenPySequence_GetItem andPySequence_GetItem, where the "fast" form doesn't handle negative indexes.
Doc/c-api/sequence.rst Outdated
| :c:func:`PySequence_Fast_GET_SIZE` is faster because it can assume *o* is a list | ||
| or tuple. | ||
| :c:func:`PySequence_Fast` and that *o* is not *NULL*. Equivalent to | ||
| :c:func:`PySequence_Size` but faster. |
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 don't like to say these are equivalent, sincePySequence_Fast_GET_SIZE has the assumption thatPySequence_Fast has been called. The old sentence was clearer.
Doc/c-api/sequence.rst Outdated
| ``PySequence_Fast*`` family of functions. On CPython, if *o* is already a | ||
| sequence or list, it will be returned, but this is an implementation detail. | ||
| Returns *NULL* on failure. If the object is not a sequence or iterable, | ||
| raises :exc:`TypeError` with *m* as the message text. |
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 suggest only describing the abstract semantics (the first and last sentences) of this function in the first paragraph and moving the implementation details below (the middle sentence).
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.
This partially duplicates the first paragraph now.
Doc/c-api/sequence.rst Outdated
| Returns *NULL* on failure. If the object is not a sequence or iterable, | ||
| raises :exc:`TypeError` with *m* as the message text. | ||
| This family of functions is labelled *Fast* since it can assume *o* is a |
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 would be clearer to say "ThePySequence_Fast_* functions are thus named because they assume...".
bedevere-bot commentedSep 12, 2019
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
I have made the requested changes; please review again. |
bedevere-bot commentedSep 12, 2019
Thanks for making the requested changes! @benjaminp: please review the changes made to this pull request. |
Doc/c-api/sequence.rst Outdated
| iterable, raises :exc:`TypeError` with *m* as the message text. | ||
| The ``PySequence_Fast*`` functions are thus named because they can assume | ||
| *o* is a :c:type:`PyTupleObject` or a :c:type:`PyListObject`, and accesses |
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.
s/accesses/access/
Doc/c-api/sequence.rst Outdated
| ``PySequence_Fast*`` family of functions. If the object is not a sequence or | ||
| iterable, raises :exc:`TypeError` with *m* as the message text. | ||
| The ``PySequence_Fast*`` functions are thus named because they can assume |
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.
removecan
Doc/c-api/sequence.rst Outdated
| The ``PySequence_Fast*`` functions are thus named because they can assume | ||
| *o* is a :c:type:`PyTupleObject` or a :c:type:`PyListObject`, and accesses | ||
| the type's data fields directly. |
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.
s/type's/*o*'s/
Doc/c-api/sequence.rst Outdated
| ``PySequence_Fast*`` family of functions. On CPython, if *o* is already a | ||
| sequence or list, it will be returned, but this is an implementation detail. | ||
| Returns *NULL* on failure. If the object is not a sequence or iterable, | ||
| raises :exc:`TypeError` with *m* as the message text. |
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.
This partially duplicates the first paragraph now.
I have made the requested changes; please review again |
bedevere-bot commentedSep 12, 2019
Thanks for making the requested changes! @benjaminp: please review the changes made to this pull request. |
Thanks@mattip for the PR, and@benjaminp for merging it 🌮🎉.. I'm working now to backport this PR to: 2.7, 3.7, 3.8. |
bedevere-bot commentedSep 12, 2019
GH-16068 is a backport of this pull request to the3.8 branch. |
bedevere-bot commentedSep 12, 2019
GH-16069 is a backport of this pull request to the3.7 branch. |
Sorry,@mattip and@benjaminp, I could not cleanly backport this to |
(cherry picked from commit57b7dbc)Co-authored-by: Matti Picus <matti.picus@gmail.com>
(cherry picked from commit57b7dbc)Co-authored-by: Matti Picus <matti.picus@gmail.com>
(cherry picked from commit57b7dbc)Co-authored-by: Matti Picus <matti.picus@gmail.com>
PySequence_Fast()should always be used (and the result checked) before any of the otherPySequence_Fast*family of functions. As an implementation detail, CPython can circumvent this ifPyTuple_CheckExact()passes, but that does not work on PyPy.xrefnumpy/numpy#12524