MAINT: reduce void type repr code duplication #11830
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
eric-wieser
reviewed
Aug 29, 2018
|
|
||
| return PyObject_CallFunction(reprfunc, "O", self); |
There was a problem hiding this comment.
I'd be inclined just to extract the contents of this if statement into a C-side helper function _void_scalar_repr- we might want to have different behavior for str and repr of a structured void in future:
static PyObject *
_void_scalar_repr(PyObject *obj) {
static PyObject *reprfunc = NULL;
npy_cache_import("numpy.core.arrayprint",
"_void_scalar_repr", &reprfunc);
if (reprfunc == NULL) {
return NULL;
}
return PyObject_CallFunction(reprfunc, "O", obj);
}There was a problem hiding this comment.
And then
static PyObject *
voidtype_str(PyObject *self)
{
PyVoidScalarObject *s = (PyVoidScalarObject*) self;
if (PyDataType_HASFIELDS(s->descr)) {
return _void_scalar_repr(self);
}
return _void_to_hex(s->obval, s->descr->elsize, "b'", "\\x", "'");
}
I'd prefer for every void member function to have an if (PyDataType_HASFIELDS ...) branch to reinforce the idea that structured types and raw-byte types should be two separate things, and at some point we should try and split void into two pieces
* voidtype_str and voidtype_repr previously had nearly-identical code; the shared code has now been abstracted to a handler function
182fbef to
efb09b1
Compare
|
Thanks Eric, revised accordingly (I hope). |
eric-wieser
approved these changes
Aug 29, 2018
|
Thanks! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The void type
__str__and__repr__handling code at the C level was almost completely identical in their respective C functions. Both already called common__repr__handling code at the Python level, and this PR aims to similarly reduce code duplication / maintenance burden at the C level.Note that it is, of course, not uncommon for
__str__implementations to simply call__repr__, and the quaternion dtype implementation uses exact duplicate code for both instead of one calling the other.Here, both functions weren't quite identical, so the common handler function accepts arguments to allow the formatting differences between
__repr__and__str__to be specified while abstracting the common logic.void type discussion / work in PR from @ahaldane : #8981