MAINT: Use a simpler return convention for internal functions#15284
MAINT: Use a simpler return convention for internal functions#15284seberg merged 3 commits intonumpy:masterfrom
Conversation
Previously these used the `PyArg_ParseTuple` converter interfaces, rather than the more typical "return null on error" convention. Switching to the latter internally makes everything a lot simpler.
| if (*at == NULL && !PyErr_Occurred()) { | ||
| _report_generic_error(); | ||
| } |
There was a problem hiding this comment.
unfortunately this really is reachable at the moment - a future patch will attempt to track down the cases when this happens, and emit the error in a more appropriate place.
seberg
left a comment
There was a problem hiding this comment.
LGTM generally, I think we may create a merge conflict here with the Py3K cleanups before this is finished though. A few some corrections (or questions).
| tmp = PyUnicode_FromEncodedObject(obj, "ascii", "strict"); | ||
| PyObject *tmp = PyUnicode_FromEncodedObject(obj, "ascii", "strict"); | ||
| if (tmp == NULL) { | ||
| goto fail; |
There was a problem hiding this comment.
| goto fail; | |
| return NULL; |
This seems like it was an error, unless setting a new error when one already is set is always OK (I thought it was not on some python versions at least). So maybe we should just clear the error. But maybe its also fine as is.
There was a problem hiding this comment.
setting a new error when one already is set is always OK
I'm pretty sure we do it all over the place when replacing an exception with a different one. I'd rather not add that to the pile of bugs I need to think about in this patch.
So maybe we should just clear the error.
I thought about this too, but decided I'd rather leave things the way they currently are for these cleanup patches, and only start fixing them if I get around to improving the error messages.
The change you suggest would result in a UnicodeDecodeError instead of a TypeError, which probably isn't helpful to users.
There was a problem hiding this comment.
Yeah, the DeprecationWarning was what tripped me first. Just keep it as is, for a moment I was just wondering if you changed it earlier accidentally, but doubt that.
There was a problem hiding this comment.
Nope, this was here from the beginning, and was of the things I was deliberately steering clear of.
| @@ -1711,36 +1687,32 @@ _convert_from_bytes(PyObject *obj, PyArray_Descr **at) | |||
| } | |||
There was a problem hiding this comment.
The line just one above seems also wrong. Maybe an error in our cleanup, maybe earlier, not sure, but it should be a return NULL; here. I assume we do not want to replace deprecation warnings when they are raced (normally). If replacing it trips tests, maybe keep it though.
There was a problem hiding this comment.
Agreed, this looked fishy - but I wanted this patch to not change any semantics.
|
Fine with merging the Py3K stuff first, I'm not moving much code around here any more so the conflicts should be minor |
Co-Authored-By: Sebastian Berg <sebastian@sipsolutions.net>
|
CI passing - did you want to get this in before or after NPY3k stuff @seberg? |
|
The other one is already rebased once, I think I will skim over and merge it (should take 10 minutes). |
|
Ok, lets put this in. Thanks Eric! |
…15284) Previously these used the `PyArg_ParseTuple` converter interfaces, rather than the more typical "return null on error" convention. Switching to the latter internally makes everything a lot simpler.
Previously these used the
PyArg_ParseTupleconverter interfaces, rather than the more typical "return null on error" convention.Switching to the latter internally makes everything a lot simpler.
Based on #15265, but not the last from this series