gh-111389: expose _PyHASH_INF/BITS/MODULUS/IMAG macros as public

[skirpichev]

• Issue: Consider restoring _PyHASH_INF/BITS/MODULUS/IMAG as public #111389

[vstinner]

I would prefer to make them public if they are useful.

[skirpichev]

I would prefer to make them public if they are useful.

@vstinner, 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)?

[vstinner]

Please don't document private APIs.

[bedevere-app]

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

[vstinner]

If you want to document these constants, make them public.

[ngoldbaum]

It looks like numpy uses _Py_HashDouble to hash numpy scalars. If we're going to expose these symbols again, is there any appetite for exposing the functions used by them in the unstable API? If not it looks like it should be straightforward to vendor the code assuming there aren't any hidden issues for making the hash values of numpy scalars possibly differ from python scalars in the future.

[skirpichev]

If you want to document these constants, make them public.

Ok, I've added PyUnstable prefixes. If that's ok, I'll add a news entry.

The other option is to revert to 0ce377a. I don't think we could just remove underscores, given the _PyHASH_NAN example.

I have made the requested changes; please review again

BTW, maybe we shouldn't export _PyHASH_BITS. This parameter is not available in sys.hash_info and examples for hashing in the docs show how to avoid using one. I'll try to patch gmpy2 to remove it's usage (seems to be trivial for mpq, but may be less efficient for mpfr). I think same could be done for the Sage.

It looks like numpy uses _Py_HashDouble to hash numpy scalars.

@ngoldbaum FYI: #91744 (comment)

[skirpichev]

I have made the requested changes; please review again

[skirpichev]

I didn't expect the Spanish Inquisition

[skirpichev]

@vstinner, not sure why bot isn't working, but I think I did requested changes.

[vstinner]

Sorry, I was struggling with #111089 this week. I will try to go through all #111481 issues next week.

[skirpichev]

BTW, maybe we shouldn't export _PyHASH_BITS.

Hmm, no, it seems we really want this, e.g.: aleaxit/gmpy#452 (see first removed commit). Then maybe we should expose this in sys.hash_info struct too?

[vstinner]

I'm working on PR #112095 which adds an "infrastructure" to test the PyHash API. I suggest waiting until this PR is merged before considering exposing these constants to the public C API.

[skirpichev]

Perhaps, I should use Py_HASH_* naming convention? (Given #112449)

[skirpichev]

@vstinner, #112449 was closed. On another hand, Py_HashPointer() function was added. Does it make sense to keep this pr?

[skirpichev]

I have made the requested changes; please review again

[bedevere-app]

Thanks for making the requested changes!

@vstinner: please review the changes made to this pull request.

[vstinner]

LGTM, I just suggest to add a comment.

@mdickinson: Would you mind to double check this PR?

[mdickinson]

Looks fine to me.

[vstinner]

Merged, thank you @skirpichev.

[skirpichev]

Thanks to all for review and patience.