Skip to content

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

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.

Sign up for GitHub

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
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
Loading

serhiy-storchaka
serhiy-storchaka reviewed Dec 15, 2018
View reviewed changes
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.

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

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

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

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 <[email protected]>
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 <[email protected]>
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 serhiy-storchaka approved these changes

@1st1 1st1 Awaiting requested review from 1st1

@asvetlov asvetlov Awaiting requested review from 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.
5 participants
@ZackerySpytz @miss-islington @bedevere-bot @serhiy-storchaka @the-knights-who-say-ni