Skip to content
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

Consider restoring _PyHASH_INF/BITS/MODULUS/IMAG as public #111389

Closed
skirpichev opened this issue Oct 27, 2023 · 13 comments
Closed

Consider restoring _PyHASH_INF/BITS/MODULUS/IMAG as public #111389

skirpichev opened this issue Oct 27, 2023 · 13 comments
Labels
topic-C-API type-feature A feature request or enhancement

Comments

@skirpichev
Copy link
Member

skirpichev commented Oct 27, 2023

This is used to implement CPython-like hashing of numeric types e.g. in Sage or in gmpy2. Actually, the same info is available via sys.hash_info, so I don't see reasons to hide this in Python.h.

See #106320 and #107026.

Linked PRs

@skirpichev skirpichev added the type-feature A feature request or enhancement label Oct 27, 2023
@sobolevn
Copy link
Member

CC @vstinner

@vstinner
Copy link
Member

Python 3.12 has 5 constants implemented as macros:

  • _PyHASH_BITS
  • _PyHASH_IMAG
  • _PyHASH_INF
  • _PyHASH_MODULUS
  • _PyHASH_MULTIPLIER

Significant commits related to these constants:

  • commit 985ecdc: new Include/pyhash.h header file
  • commit 27cbcd6: in Include/pyport.h, use unsigned integer, replace Py_hash_t with Py_uhash_t
  • commit 63e6c32: add _PyHASH_MULTIPLIER constant
  • commit 8035bc5: add Py_uhash_t type, replace unsigned long with size_t
  • commit dc787d2: add _PyHASH_BITS, _PyHASH_IMAG, _PyHASH_INF and _PyHASH_MODULUS constants

the same info is available via sys.hash_info, so I don't see reasons to hide this in Python.h.

Right, it makes sense to make them public. I'm not sure why it was private before.

We can keep them as macros, but maybe we should consider declaring them as variables (using with dlsym for example) to make them usable in programming languages and use cases which cannot use macros.

@vstinner
Copy link
Member

We can keep them as macros, but maybe we should consider declaring them as variables (using with dlsym for example) to make them usable in programming languages and use cases which cannot use macros.

I created capi-workgroup/api-evolution#37 to discuss that.

@skirpichev
Copy link
Member Author

@vstinner, I did a quick reversion in #111418

skirpichev added a commit to skirpichev/cpython that referenced this issue Oct 28, 2023
@vstinner
Copy link
Member

Should we also add some functions like Py_HashPointer()?

@vstinner
Copy link
Member

vstinner commented Oct 28, 2023

@skirpichev:

are you about documenting these macroses somewhere (I propose https://docs.python.org/3/library/sys.html#sys.hash_info, where we could mention them in descriptions for different fields) AND about renaming them too (add PyUnstable_ aliases)?

If constants are made public, yes, they must be documented.

I don't think that PyUnstable prefix is needed, these constants seem stable.

@skirpichev
Copy link
Member Author

Should we also add some functions like Py_HashPointer()?

Maybe others (I don't know examples), but not this function. It's used currently in the gmpy2, but this could be replaced by PyBaseObject_Type.tp_hash. And this is a documented replacement:

if math.isnan(x):
return object.__hash__(x)

If constants are made public, yes, they must be documented.

Ok, I'll go with suggested above sys.hash_info. Let me know if you have in mind a better place to put the docs.

these constants seem stable

Yep. But can be removed in any minor release, as _PyHASH_NAN.

@vstinner
Copy link
Member

https://docs.python.org/dev/library/sys.html is the Python API.

For the C API, I would prefer to create a new page under https://docs.python.org/dev/c-api/ category.

@skirpichev
Copy link
Member Author

Should we also add some functions like Py_HashPointer()?

UPD:

The one drawback with the PyBaseObject_Type.tp_hash solution above is that there is no guarantee that this function does accept an arbitrary pointer (not only a pointer to some PyObject subclass). (In gmpy2 we pass mpfr_t: https://github.com/aleaxit/gmpy/blob/e002e2c28994f6c9edd411c1f1e09d0bbbbfca9b/src/gmpy2_hash.c#L122)

On one hand, this is unlikely to be changed. On another hand, this situation is not documented. Maybe it should?

@vstinner
Copy link
Member

I restored _PyHASH constants with PR #112115.

@vstinner
Copy link
Member

Should we also add some functions like Py_HashPointer()?

I proposed PR #112096 to add Py_HashPointer() function.

@vstinner
Copy link
Member

@skirpichev: Once a public Py_HashDouble() function will be added, I will look into this issue: #111545

vstinner added a commit that referenced this issue Mar 9, 2024
@vstinner vstinner closed this as completed Mar 9, 2024
@vstinner
Copy link
Member

vstinner commented Mar 9, 2024

This is used to implement CPython-like hashing of numeric types e.g. in Sage or in gmpy2.

You can now propose changes for these projects to use the public names :-) I added the constants to pythoncapi-compat: python/pythoncapi-compat@7539c7f

adorilson pushed a commit to adorilson/cpython that referenced this issue Mar 25, 2024
diegorusso pushed a commit to diegorusso/cpython that referenced this issue Apr 17, 2024
vstinner added a commit to vstinner/cpython that referenced this issue May 20, 2024
vstinner added a commit to vstinner/cpython that referenced this issue May 20, 2024
vstinner added a commit to vstinner/cpython that referenced this issue May 20, 2024
vstinner added a commit to vstinner/cpython that referenced this issue May 20, 2024
miss-islington pushed a commit to miss-islington/cpython that referenced this issue May 21, 2024
(cherry picked from commit f6da790)

Co-authored-by: Victor Stinner <[email protected]>
vstinner added a commit that referenced this issue Jun 4, 2024
gh-111389: Add PyHASH_MULTIPLIER constant (GH-119214)
(cherry picked from commit f6da790)

Co-authored-by: Victor Stinner <[email protected]>
estyxx pushed a commit to estyxx/cpython that referenced this issue Jul 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic-C-API type-feature A feature request or enhancement
Projects
None yet
Development

No branches or pull requests

4 participants