BUG: core: fix NPY_TITLE_KEY macro on pypy #10860
Conversation
b76ad0c to
adb3997
Compare
| #endif | ||
| return 0; | ||
| } | ||
| #define NPY_TITLE_KEY(key, value) (NPY_TITLE_KEY_check(key, value)) |
There was a problem hiding this comment.
Why the macro wrapping? Should both paths maybe be replaced with the inline function, which would narrow the scope of the PYPY check?
There was a problem hiding this comment.
Because there's code that does
if NPY_TITLE_KEY(key, value) { ... and omits the parentheses, and I wanted to keep this PR minimal and obviously correct for non-pypy.
There was a problem hiding this comment.
Updated with comments explaining why.
On Pypy, dictionary keys do not necessarily preserve object identity. This however was assumed by the NPY_TITLE_KEY macro, which relies on descriptor.c:568 using the same 'title' object both as a dictionary key as an entry in the tuple inserted. Since the items in the field dict are unique, value identity is however sufficient for the NPY_TITLE_KEY macro. On PyPy, fix the macro by comparing values instead.
adb3997 to
18fd6a4
Compare
|
LGTM |
|
IMO this should be in 1.15, does it justify an entry in the release notes? |
|
We mostly use the release notes for enhancements/deprecations/signature_changes -- basically things that change intended behavior or add options. Bug fixes for old/minor problems don't count in that regard. Newly introduced bugs that everyone notices are a different matter. |
|
Thanks @pv. |
On Pypy, dictionary keys do not necessarily preserve object identity.
This however was assumed by the NPY_TITLE_KEY macro, which relies on
descriptor.c:568 using the same 'title' object both as a dictionary key
as an entry in the tuple inserted.
Since the items in the field dict are unique, value identity is however
sufficient for the NPY_TITLE_KEY macro. On PyPy, fix the macro by
comparing values instead.
cf. https://bitbucket.org/pypy/pypy/issues/2789
cc: @mattip