Skip to content

MAINT: Ensure _convert_from_* functions set errors - #15310

Merged
mattip merged 1 commit into
numpy:masterfrom
eric-wieser:tidy-dtype-ctors-fix-errors-wip
Jan 11, 2020
Merged

mattip merged 1 commit into
numpy:masterfrom
eric-wieser:tidy-dtype-ctors-fix-errors-wip

Conversation

@eric-wieser

@eric-wieser eric-wieser commented Jan 10, 2020 •

Copy link
Copy Markdown
Member

Previously some of these would return null without setting an error, meaning the caller had to clean up after them.

Removes all direct callers of PyArray_DescrConverter in this file, since _convert_from_any has less indirection and a simpler calling convention.


Follow-up work I'll file an issue for if this goes in - replace each call to _report_generic_error with a useful error message. (done in #15316)

@eric-wieser
eric-wieser force-pushed the tidy-dtype-ctors-fix-errors-wip branch 2 times, most recently from 95c85aa to b572dc9 Compare January 10, 2020 18:28
@eric-wieser
eric-wieser force-pushed the tidy-dtype-ctors-fix-errors-wip branch from b572dc9 to 3ec6837 Compare January 10, 2020 18:42
Previously some of these would return null without setting an error, meaning the caller had to clean up after them.

Removes all direct callers of `PyArray_DescrConverter` in this file, since `_convert_from_any` has less indirection and a simpler calling convention.
@eric-wieser
eric-wieser force-pushed the tidy-dtype-ctors-fix-errors-wip branch from 3ec6837 to 4cd5a88 Compare January 10, 2020 18:54
PyArray_Descr *newdescr;
int ret = PyArray_DescrConverter(dtypedescr, &newdescr);

PyArray_Descr *newdescr = _convert_from_any(dtypedescr, 0);

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.

I could make this respect alignment by adding an align argument to _try_convert_from_dtype_attr, but we'd then need to plumb that through PyArray_DescrFromTypeObject too, and the whole thing wouldn't make much sense anyway - user types are unlikely to have separate aligned and unaligned variants.

At any rate, this matches the old behavior.

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.

OK, lets keep old behaviours for now in any case. If you wanted, we could add a comment, or even issue. But my guess is that it is not worth the trouble right now.

if (retcode == NPY_FAIL) {
res = NULL;
}
res = _convert_from_any(PyList_GET_ITEM(listobj, 0), align);

@eric-wieser eric-wieser Jan 10, 2020 •

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 fixes an extension of the bug in similar nonsensical code to #15287 (comment), namely

np.typeDict['mypoint'] = [('a', 'i1'), ('b', 'i2')]
assert np.dtype('mypoint,', align=True).isalignedstruct

(xref gh-15296)

@eric-wieser
eric-wieser marked this pull request as ready for review January 10, 2020 19:07
@eric-wieser
eric-wieser requested a review from seberg January 10, 2020 19:26

@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.

LGTM, I may have another look, but first check, cannot find any missing generic error setting or other issue.

PyArray_Descr *newdescr;
int ret = PyArray_DescrConverter(dtypedescr, &newdescr);

PyArray_Descr *newdescr = _convert_from_any(dtypedescr, 0);

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.

OK, lets keep old behaviours for now in any case. If you wanted, we could add a comment, or even issue. But my guess is that it is not worth the trouble right now.

@mattip
mattip merged commit 80fc0f5 into numpy:master Jan 11, 2020
@mattip

mattip commented Jan 11, 2020

Copy link
Copy Markdown
Member

Thanks @eric-wieser

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