Skip to content

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

Merged
Skylion007 merged 1 commit into
pybind:masterfrom
Skylion007:pylist-append-error-checking
Oct 3, 2022
Merged

bugfix: Add error checking to list append and insert#4208
Skylion007 merged 1 commit into
pybind:masterfrom
Skylion007:pylist-append-error-checking

Conversation

@Skylion007

@Skylion007Skylion007 commented Oct 3, 2022

Copy link
Copy Markdown
Collaborator

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.

rwgk
rwgk approved these changes Oct 3, 2022

@rwgkrwgk left a comment

Copy link
Copy Markdown
Collaborator

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
Copy Markdown
CollaboratorAuthor

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.

rwgk
rwgk approved these changes Oct 3, 2022

@rwgkrwgk left a comment

Copy link
Copy Markdown
Collaborator

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:masterOct 3, 2022
@Skylion007
Skylion007 deleted the pylist-append-error-checking branch October 3, 2022 17:44
@github-actionsgithub-actionsBot added the needs changelog Possibly needs a changelog entry label Oct 3, 2022
@henryiiihenryiii removed the needs changelog Possibly needs a changelog entry label Oct 20, 2022
@rwgkrwgk mentioned this pull request Feb 11, 2023
Sign up for freeto 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

@Skylion007@rwgk@henryiii