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

Jump to bottom

bpo-35504: Fix segfaults and SystemErrors when deleting certain attrs #11175

Merged
merged 3 commits into from Dec 17, 2018
Merged

bpo-35504: Fix segfaults and SystemErrors when deleting certain attrs #11175

merged 3 commits into from Dec 17, 2018

Conversation

ZackerySpytz
Copy link
Contributor

@ZackerySpytz ZackerySpytz commented Dec 15, 2018 •
edited by bedevere-bot

@ZackerySpytz ZackerySpytz requested review from 1st1 and asvetlov as code owners Dec 15, 2018
serhiy-storchaka
serhiy-storchaka reviewed Dec 15, 2018
View changes
Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

LGTM in general. Just few suggestions:

  • It is more common to start the error message from a lower case letter (unless it consists of several sentences). I.e. use "cannot" instead of "Cannot".

  • It may be worth to check that the attribute exists, i.e. reading it does not fail.

  • For underscored attributes, it is better to move tests to test classes specific for the C accelerators. If the pure Python version contains corresponding attributes, deleting them will not raise an AttributeError. If it doe not contain them, deleting them will fail for different reason.

@ZackerySpytz
Copy link
Contributor Author

ZackerySpytz commented Dec 17, 2018

Thank you, @serhiy-storchaka, for the comments.

@serhiy-storchaka serhiy-storchaka added type-bug An unexpected behavior, bug, or error needs backport to 3.7 labels Dec 17, 2018
@serhiy-storchaka serhiy-storchaka merged commit 842acaa into python:master Dec 17, 2018
@miss-islington
Copy link
Contributor

miss-islington commented Dec 17, 2018

Thanks @ZackerySpytz for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖

@bedevere-bot
Copy link

bedevere-bot commented Dec 17, 2018

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

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Dec 17, 2018
@ZackerySpytz @miss-islington
bpo-35504: Fix segfaults and SystemErrors when deleting certain attrs. (
7ffc4c2 
pythonGH-11175)

(cherry picked from commit 842acaa)

Co-authored-by: Zackery Spytz <zspytz@gmail.com>
miss-islington added a commit that referenced this pull request Dec 17, 2018
@miss-islington @ZackerySpytz
bpo-35504: Fix segfaults and SystemErrors when deleting certain attrs. (
cb27284 
GH-11175)

(cherry picked from commit 842acaa)

Co-authored-by: Zackery Spytz <zspytz@gmail.com>
ZackerySpytz added a commit to ZackerySpytz/cpython that referenced this pull request Dec 20, 2018
@ZackerySpytz
bpo-35504: Fix segfaults and SystemErrors when deleting certain attrs. (
360e626 
pythonGH-11175)

(cherry picked from commit 842acaa)
serhiy-storchaka pushed a commit that referenced this pull request Dec 20, 2018
@ZackerySpytz @serhiy-storchaka
bpo-35504: Fix segfaults and SystemErrors when deleting certain attrs. (
f347c6e 
GH-11175) (GH-11249)

(cherry picked from commit 842acaa)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@serhiy-storchaka serhiy-storchaka

@1st1 1st1

@asvetlov asvetlov

Assignees
No one assigned
Labels
type-bug An unexpected behavior, bug, or error
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@ZackerySpytz @miss-islington @bedevere-bot @serhiy-storchaka @the-knights-who-say-ni