Skip to content

bpo-39453: Fix contains method of list to hold strong references#18181

Merged
pablogsal merged 3 commits into
python:masterfrom
corona10:bpo-39453
Jan 27, 2020
Merged

bpo-39453: Fix contains method of list to hold strong references#18181
pablogsal merged 3 commits into
python:masterfrom
corona10:bpo-39453

Conversation

@corona10

@corona10 corona10 commented Jan 25, 2020 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Member

@corona10 corona10 changed the title bpo-39453: Fix contains method of list to hold strong references [WIP] bpo-39453: Fix contains method of list to hold strong references Jan 25, 2020
@corona10 corona10 changed the title [WIP] bpo-39453: Fix contains method of list to hold strong references bpo-39453: Fix contains method of list to hold strong references Jan 25, 2020
@corona10

corona10 commented Jan 25, 2020

Copy link
Copy Markdown
Member Author

@pablogsal @serhiy-storchaka @vstinner
All test are passed :)

rhettinger
rhettinger reviewed Jan 26, 2020
View reviewed changes
Comment thread Objects/listobject.c Outdated
Comment thread Misc/NEWS.d/next/Core and Builtins/2020-01-25-23-51-17.bpo-39453.xCOkYk.rst Outdated
corona10
corona10 commented Jan 26, 2020
View reviewed changes

@corona10 corona10 left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@rhettinger I've updated the PR Please take a look :)

@corona10
corona10 requested a review from rhettinger January 26, 2020 05:51
serhiy-storchaka
serhiy-storchaka reviewed Jan 27, 2020
View reviewed changes
Comment thread Misc/NEWS.d/next/Core and Builtins/2020-01-25-23-51-17.bpo-39453.xCOkYk.rst Outdated
@@ -0,0 +1,2 @@
Fix contains method of list to hold strong references of elements while

@serhiy-storchaka serhiy-storchaka Jan 27, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It describes the implementation, but says nothing to an average Python user. I would formulate the entry in terms of Python: "Fixed possible crash in ``list.contains` when a list is changed during comparing items."

@corona10 corona10 Jan 27, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Updated!

pablogsal
pablogsal reviewed Jan 27, 2020
View reviewed changes
Comment thread Lib/test/test_list.py Outdated
pablogsal
pablogsal reviewed Jan 27, 2020
View reviewed changes
Comment thread Misc/NEWS.d/next/Core and Builtins/2020-01-25-23-51-17.bpo-39453.xCOkYk.rst Outdated
serhiy-storchaka
serhiy-storchaka approved these changes Jan 27, 2020
View reviewed changes

@serhiy-storchaka serhiy-storchaka left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM with changes proposed by @pablogsal. 👍

corona10
corona10 commented Jan 27, 2020
View reviewed changes

@corona10 corona10 left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@pablogsal @serhiy-storchaka
Thanks for the review :)
I 've updated them

@corona10
corona10 requested a review from pablogsal January 27, 2020 12:53
@pablogsal
pablogsal merged commit 4dbf2d8 into python:master Jan 27, 2020
@miss-islington

miss-islington commented Jan 27, 2020

Copy link
Copy Markdown
Contributor

Thanks @corona10 for the PR, and @pablogsal for merging it 🌮🎉.. I'm working now to backport this PR to: 3.6, 3.7, 3.8.
🐍🍒⛏🤖

@miss-islington

miss-islington commented Jan 27, 2020

Copy link
Copy Markdown
Contributor

Sorry, @corona10 and @pablogsal, I could not cleanly backport this to 3.8 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 4dbf2d8c6789a9b7299b142033073213604b8fdc 3.8

@miss-islington

miss-islington commented Jan 27, 2020

Copy link
Copy Markdown
Contributor

Sorry @corona10 and @pablogsal, I had trouble checking out the 3.7 backport branch.
Please backport using cherry_picker on command line.
cherry_picker 4dbf2d8c6789a9b7299b142033073213604b8fdc 3.7

@miss-islington

miss-islington commented Jan 27, 2020

Copy link
Copy Markdown
Contributor

Sorry, @corona10 and @pablogsal, I could not cleanly backport this to 3.6 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 4dbf2d8c6789a9b7299b142033073213604b8fdc 3.6

@pablogsal

pablogsal commented Jan 27, 2020

Copy link
Copy Markdown
Member

@corona10 Could you make the backports with cherry_picker? Ping me in them and I will do the merge

@corona10 corona10 mentioned this pull request Jan 27, 2020
Merged
corona10 added a commit to corona10/cpython that referenced this pull request Jan 27, 2020
@corona10
bpo-39453: Make list.__contains__ hold strong references to avoid cra…
36978c1 
…shes (pythonGH-18181)
corona10 added a commit to corona10/cpython that referenced this pull request Jan 27, 2020
@corona10
bpo-39453: Make list.__contains__ hold strong references to avoid cra…
9d3e26d 
…shes (pythonGH-18181)
shihai1991 pushed a commit to shihai1991/cpython that referenced this pull request Jan 31, 2020
@corona10 @shihai1991
bpo-39453: Make list.__contains__ hold strong references to avoid cra…
9967763 
…shes (pythonGH-18181)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rhettinger rhettinger rhettinger approved these changes
@serhiy-storchaka serhiy-storchaka serhiy-storchaka approved these changes
@pablogsal pablogsal pablogsal approved these changes
@vstinner vstinner Awaiting requested review from vstinner

Assignees

@pablogsal pablogsal

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@corona10 @miss-islington @pablogsal @rhettinger @serhiy-storchaka @the-knights-who-say-ni @bedevere-bot