DOC: emphasize the need to always call PySequence_Fast by mattip · Pull Request #11140 · python/cpython

mattip · 2018-12-13T06:30:00Z

PySequence_Fast() should always be used (and the result checked) before any of the other PySequence_Fast* family of functions. As an implementation detail, CPython can circumvent this if PyTuple_CheckExact() passes, but that does not work on PyPy.

xref numpy/numpy#12524

serhiy-storchaka · 2018-12-13T10:35:09Z

Is not this a PyPy issue that PySequence_Fast* functions do not work with lists and tuples?

mattip · 2018-12-13T11:16:59Z

The functions work for ret after calling PyObject* ret = PySequence_Fast(o), on both PyPy and CPython, but the documentation was not clear that this is always necessary. Hopefully now it is clearer.

eric-wieser · 2018-12-14T04:12:13Z

@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 "PySequence_Fast_ITEMS is only defined on the result of PySequence_Fast".

serhiy-storchaka · 2019-06-07T04:20:56Z

I think this change needs more wide discussion. Please open an issue on the bug tracker and ask the question on the Python-Dev mailing list.

JulienPalard · 2019-09-11T10:40:43Z

@mattip Hi, have you been able to open an issue on bugs.python.org about it as requested by Serhiy?

mattip · 2019-09-11T12:11:06Z

@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?

benjaminp · 2019-09-12T14:01:46Z


 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.


Not exactly. At the very least, it has a difference analogous to the one between PySequence_GetItem and PySequence_GetItem, where the "fast" form doesn't handle negative indexes.

benjaminp · 2019-09-12T14:04:34Z

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


I don't like to say these are equivalent, since PySequence_Fast_GET_SIZE has the assumption that PySequence_Fast has been called. The old sentence was clearer.

benjaminp · 2019-09-12T14:08:44Z

+ ``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.


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

This partially duplicates the first paragraph now.

benjaminp · 2019-09-12T14:09:26Z

+ 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


It would be clearer to say "The PySequence_Fast_* functions are thus named because they assume...".

bedevere-bot · 2019-09-12T14:10:25Z

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. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

mattip · 2019-09-12T15:19:15Z

I have made the requested changes; please review again.

bedevere-bot · 2019-09-12T15:19:18Z

Thanks for making the requested changes!

@benjaminp: please review the changes made to this pull request.

benjaminp · 2019-09-12T15:24:25Z

+ 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


s/accesses/access/

benjaminp · 2019-09-12T15:24:47Z

+ ``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


remove can

benjaminp · 2019-09-12T15:25:11Z

+
+ 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.


s/type's/*o*'s/

benjaminp · 2019-09-12T15:27:12Z

+ ``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.


This partially duplicates the first paragraph now.

mattip · 2019-09-12T15:39:19Z

I have made the requested changes; please review again

bedevere-bot · 2019-09-12T15:39:21Z

Thanks for making the requested changes!

@benjaminp: please review the changes made to this pull request.

miss-islington · 2019-09-12T16:19:09Z

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 · 2019-09-12T16:19:24Z

GH-16068 is a backport of this pull request to the 3.8 branch.

bedevere-bot · 2019-09-12T16:19:34Z

GH-16069 is a backport of this pull request to the 3.7 branch.

miss-islington · 2019-09-12T16:19:38Z

Sorry, @mattip and @benjaminp, I could not cleanly backport this to 2.7 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 57b7dbc46e71269d855e644d30826d33eedee2a1 2.7

(cherry picked from commit 57b7dbc) Co-authored-by: Matti Picus <matti.picus@gmail.com>

DOC: emphasize the need to always call PySequence_Fast
d590938

the-knights-who-say-ni added the CLA signed label Dec 13, 2018

bedevere-bot added docs Documentation in the Doc dir awaiting review labels Dec 13, 2018

mattip mentioned this pull request Dec 13, 2018
BUG: fix improper use of C-API numpy/numpy#12524
Merged

jdemeyer approved these changes Feb 4, 2019
View reviewed changes

bedevere-bot added awaiting core review and removed awaiting review labels Feb 4, 2019

csabella requested review from benjaminp and serhiy-storchaka June 5, 2019 23:22

benjaminp requested changes Sep 12, 2019
View reviewed changes

bedevere-bot added awaiting changes and removed awaiting core review labels Sep 12, 2019

DOC: changes from review
f4a8052

bedevere-bot added awaiting change review and removed awaiting changes labels Sep 12, 2019

bedevere-bot requested a review from benjaminp September 12, 2019 15:19

benjaminp reviewed Sep 12, 2019
View reviewed changes

DOC: changes from review
04a8822

bedevere-bot requested a review from benjaminp September 12, 2019 15:39

benjaminp added 2 commits September 12, 2019 17:03

tweak
b1a77ed

delete comma
01e1486

benjaminp added needs backport to 2.7 skip issue skip news labels Sep 12, 2019

benjaminp merged commit 57b7dbc into python:masterSep 12, 2019

bedevere-bot removed the awaiting change review label Sep 12, 2019

bedevere-bot removed needs backport to 3.8 labels Sep 12, 2019

miss-islington assigned benjaminp Sep 12, 2019

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Sep 12, 2019
Emphasize the need to always call PySequence_Fast. (pythonGH-11140)
37c83f1
(cherry picked from commit 57b7dbc) Co-authored-by: Matti Picus <matti.picus@gmail.com>

benjaminp removed the needs backport to 2.7 label Sep 12, 2019

miss-islington added a commit that referenced this pull request Sep 12, 2019
Emphasize the need to always call PySequence_Fast. (GH-11140)
6e0c066
(cherry picked from commit 57b7dbc) Co-authored-by: Matti Picus <matti.picus@gmail.com>

miss-islington added a commit that referenced this pull request Sep 12, 2019
Emphasize the need to always call PySequence_Fast. (GH-11140)
4145f62
(cherry picked from commit 57b7dbc) Co-authored-by: Matti Picus <matti.picus@gmail.com>

gpshead added a commit to gpshead/cpython that referenced this pull request Feb 12, 2024
Merge branch 'main' into fixup/pythongh-11140
4859a9b

Uh oh!

Uh oh!

Conversation

mattip commented Dec 13, 2018

Uh oh!

serhiy-storchaka commented Dec 13, 2018

Uh oh!

mattip commented Dec 13, 2018

Uh oh!

eric-wieser commented Dec 14, 2018

Uh oh!

serhiy-storchaka commented Jun 7, 2019

Uh oh!

JulienPalard commented Sep 11, 2019

Uh oh!

mattip commented Sep 11, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Sep 12, 2019

Uh oh!

mattip commented Sep 12, 2019

Uh oh!

bedevere-bot commented Sep 12, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattip commented Sep 12, 2019

Uh oh!

bedevere-bot commented Sep 12, 2019

Uh oh!

miss-islington commented Sep 12, 2019

Uh oh!

bedevere-bot commented Sep 12, 2019

Uh oh!

bedevere-bot commented Sep 12, 2019

Uh oh!

miss-islington commented Sep 12, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants