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

bugfix: Add error checking to list append and insert #4208

Merged
merged 1 commit into from
Oct 3, 2022

Conversation

Skylion007
Copy link
Collaborator

@Skylion007 Skylion007 commented Oct 3, 2022

Description

I was looking through our python bindings and noticed we didn't do any error checking for insert or append in the list object in pytypes.h. While the only way append or insert can fail if is the list is unable to grow (allocation failure)This change will properly propagate exceptions for both these methods in this rare case.

Suggested changelog entry:

* (bugfix): Add proper error checking to C++ bindings for Python list append and insert.

@Skylion007 Skylion007 requested review from rwgk and henryiii October 3, 2022 14:56
Copy link
Collaborator

@rwgk rwgk left a comment

Choose a reason for hiding this comment

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

For .insert(): wow, how did this go unnoticed for so long? Great catch!

It would be ideal to add a test for the .insert() failure. I guess it's just a line or two.

@Skylion007
Copy link
Collaborator Author

For .insert(): wow, how did this go unnoticed for so long? Great catch!

It would be ideal to add a test for the .insert() failure. I guess it's just a line or two.

Whelp, I was wrong.

a = []
a.insert(999, 1)
a == [1,]

That is perfectly valid and does not raise any exceptions. Yeah Python! :/ So yeah, this exception will only be raised on out of memory errors then. I've updated the PR description.

Copy link
Collaborator

@rwgk rwgk left a comment

Choose a reason for hiding this comment

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

That is perfectly valid and does not raise any exceptions.

Oh! I would not have guessed.

@Skylion007 Skylion007 merged commit c78dfe6 into pybind:master Oct 3, 2022
@Skylion007 Skylion007 deleted the pylist-append-error-checking branch October 3, 2022 17:44
@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label Oct 3, 2022
@henryiii henryiii removed the needs changelog Possibly needs a changelog entry label Oct 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants