Skip to content

MAINT: Improve error handling in umathmodule setup - #17014

Merged
seberg merged 2 commits into
numpy:masterfrom
eric-wieser:malloc-check
Aug 6, 2020
Merged

seberg merged 2 commits into
numpy:masterfrom
eric-wieser:malloc-check

Conversation

@eric-wieser

Copy link
Copy Markdown
Member

Really this only ensures MemoryErrors propagate, nothing else can go wrong here.

Addresses item 3 from #17012. I don't think this is worth considering a bug, since running out of memory when creating a ufunc is not likely.

Comment thread numpy/core/src/umath/umathmodule.c Outdated
Comment on lines 257 to 258

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This list had gotten very stale - seemed better to avoid it altogether

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

True, the conditional style is not really used in NumPy, but I agree with using it here, everything else is just extremely verbose.

Really this only ensures MemoryErrors propagate, nothing else can go wrong here.

@seberg seberg left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks Eric. LGTM modulo missing (unimportant) DECREF and test success.

Comment thread numpy/core/src/umath/umathmodule.c
Comment thread numpy/core/src/umath/umathmodule.c Outdated
Comment on lines 257 to 258

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

True, the conditional style is not really used in NumPy, but I agree with using it here, everything else is just extremely verbose.

@seberg

seberg commented Aug 6, 2020

Copy link
Copy Markdown
Member

Thanks Eric, I took the liberty of just adding the DECREF.

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.

3 participants