-
-
Notifications
You must be signed in to change notification settings - Fork 12.2k
BUG,ENH: fix pickling user-scalars by allowing non-format buffer export #17295
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
d92f151
83b517b
fae94b7
1725f31
2fc4dbb
e9ce0f4
63f90a8
09934b2
d02ca96
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -35,7 +35,7 @@ scalar_value(PyObject *scalar, PyArray_Descr *descr) | |
| { | ||
| int type_num; | ||
| int align; | ||
| npy_intp memloc; | ||
| uintptr_t memloc; | ||
| if (descr == NULL) { | ||
| descr = PyArray_DescrFromScalar(scalar); | ||
| type_num = descr->type_num; | ||
|
|
@@ -168,7 +168,7 @@ scalar_value(PyObject *scalar, PyArray_Descr *descr) | |
| * Use the alignment flag to figure out where the data begins | ||
| * after a PyObject_HEAD | ||
| */ | ||
| memloc = (npy_intp)scalar; | ||
| memloc = (uintptr_t)scalar; | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seems like this casting to signed was the problem (and I first thought it can't be because the denominator is stored as a If the value is too large, it would be a negative which breaks the rounding (and also means that a previously aligned value looks unaligned). |
||
| memloc += sizeof(PyObject); | ||
| /* now round-up to the nearest alignment value */ | ||
| align = descr->alignment; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, this looks risky - are you sure it shouldn't be:
Otherwise
NULLis considered equal to arbitrary strings, and equality is not transitiveThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, should be correct. if the format is
NULLit seems OK to replace it with any other format. Now if the old (first) format isNULL, we will replace theNULLwith the actual (second) format. If the second format isNULL, format is ignored completely, so it is fine as well.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An empty string would indicate that the itemsize is 0 in the exported buffer, I guess. A size change could seem problematic, but I do not think so.
In theory, changing the itemsize might be dangerous, but it is explicitly stored in the exported buffer info, so while it could be dangerous from a buffer user perspective, I do not think it is dangerous for having incorrect information inside the exported buffer information.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where does this replacing of
NULLhappen?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/numpy/numpy/pull/17295/files#diff-761c5d4c611d2cd411341182fcee883dR662