Skip to content

Comments

BUG: check for errors after PyArray_DESCR_REPLACE#12546

Merged
seberg merged 1 commit intonumpy:masterfrom
mattip:dtype-err-check
Dec 17, 2018
Merged

BUG: check for errors after PyArray_DESCR_REPLACE#12546
seberg merged 1 commit intonumpy:masterfrom
mattip:dtype-err-check

Conversation

@mattip
Copy link
Member

@mattip mattip commented Dec 14, 2018

PyArray_DESCR_REPLACE(descr) is a macro that calls PyArray_DescrNew. Since PyArray_DescrNew can error, we must check the return value.

Refactored out of #12430

@seberg
Copy link
Member

seberg commented Dec 14, 2018

Looks good to me, is there an easy way to test these?

Copy link
Member

Choose a reason for hiding this comment

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

This should be goto error, not goto fail.

As is, it silences the MemoryError that might occur here, and tries to replace it with another exception

Copy link
Member

Choose a reason for hiding this comment

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

Or perhaps even just return NPY_FAIL

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

Copy link
Member

@eric-wieser eric-wieser Dec 17, 2018

Choose a reason for hiding this comment

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

error here too (or return NPY_FAIL)

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

@mattip
Copy link
Member Author

mattip commented Dec 17, 2018

is there an easy way to test these?

Not really. We could consider a flag that prevents memory allocation that could be turned on randomly in testing.

@seberg
Copy link
Member

seberg commented Dec 17, 2018

Should have realized it was memory allocation errors. Thanks both of you!

@seberg seberg merged commit 41d5a9f into numpy:master Dec 17, 2018
@charris charris added this to the 1.16.0 release milestone Dec 17, 2018
@charris charris removed the 09 - Backport-Candidate PRs tagged should be backported label Dec 18, 2018
@charris charris removed this from the 1.16.0 release milestone Dec 18, 2018
@mattip mattip deleted the dtype-err-check branch May 20, 2019 17:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants