gh-111389: expose _PyHASH_INF/BITS/MODULUS/IMAG macros as public by skirpichev · Pull Request #111418 · python/cpython

skirpichev · 2023-10-28T02:24:40Z

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

…public This partially reverts python#107026.

vstinner · 2023-10-28T13:10:56Z

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

skirpichev · 2023-10-28T15:36:53Z

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)?

Doc/library/sys.rst

This reverts commit 2c6002f.

vstinner

Please don't document private APIs.

bedevere-app · 2023-10-30T15:34:51Z

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 · 2023-10-30T15:51:05Z

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

ngoldbaum · 2023-10-30T19:48:43Z

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 · 2023-10-31T06:57:43Z

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 · 2023-11-03T03:40:10Z

I have made the requested changes; please review again

skirpichev · 2023-11-03T03:40:53Z

I didn't expect the Spanish Inquisition

skirpichev · 2023-11-03T03:43:01Z

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

vstinner · 2023-11-04T10:01:21Z

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

skirpichev · 2023-11-05T03:36:01Z

BTW, maybe we shouldn't export _PyHASH_BITS.

Hmm, no, it seems we really want this, e.g.: gmpy2/gmpy2#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.

Include/pyhash.h

Misc/NEWS.d/next/C API/2023-11-15-09-24-51.gh-issue-111418.FYYetY.rst

Doc/c-api/hash.rst

…etY.rst Co-authored-by: Victor Stinner <vstinner@python.org>

Was rendered: "See also the PyTypeObject.tp_hash member and Hashing of numeric types for more details about hashing of numeric types."

skirpichev · 2023-12-15T03:59:30Z

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

skirpichev · 2023-12-21T02:00:45Z

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

skirpichev · 2024-03-04T06:26:02Z

I have made the requested changes; please review again

bedevere-app · 2024-03-04T06:26:07Z

Thanks for making the requested changes!

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

Include/cpython/pyhash.h

Doc/c-api/hash.rst

Co-authored-by: Victor Stinner <vstinner@python.org>

vstinner

LGTM, I just suggest to add a comment.

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

Include/cpython/pyhash.h

mdickinson

Looks fine to me.

vstinner · 2024-03-09T20:32:18Z

Merged, thank you @skirpichev.

skirpichev · 2024-03-10T00:11:54Z

Thanks to all for review and patience.

…python#111418) Co-authored-by: Victor Stinner <vstinner@python.org>

skirpichev requested a review from tiran as a code owner October 28, 2023 02:24

bedevere-app bot added the awaiting review label Oct 28, 2023

bedevere-app bot mentioned this pull request Oct 28, 2023

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

Closed

pythongh-111389: restoring _PyHASH_INF/BITS/MODULUS/IMAG macroses as …

0ce377a

…public This partially reverts python#107026.

skirpichev force-pushed the restore-pyhash-and-pylongnew branch from 1edec34 to 0ce377a Compare October 28, 2023 03:52

Mention public macros in docs

2c6002f

skirpichev force-pushed the restore-pyhash-and-pylongnew branch from 1f9f2fc to 2c6002f Compare October 29, 2023 04:52

vstinner reviewed Oct 30, 2023

View reviewed changes

Doc/library/sys.rst Outdated Show resolved Hide resolved

skirpichev added 2 commits October 30, 2023 16:29

Revert "Mention public macros in docs"

5c42cae

This reverts commit 2c6002f.

Add Doc/c-api/hashing.rst for hash-related C macros

de4e3ae

vstinner requested changes Oct 30, 2023

View reviewed changes

bedevere-app bot added awaiting changes and removed awaiting review labels Oct 30, 2023

befeleme mentioned this pull request Oct 30, 2023

BUG: error: implicit declaration of function ‘_Py_HashDouble’ (building with Python 3.13~alpha1) numpy/numpy#25035

Closed

Expose macroses as unstable API

1a50a7b

seberg mentioned this pull request Oct 31, 2023

Make _Py_HashDouble public again as "unstable" API #111545

Closed

skirpichev requested a review from vstinner November 1, 2023 14:26

skirpichev mentioned this pull request Nov 3, 2023

Is Bedevere retired? python/bedevere#604

Closed

vstinner reviewed Nov 15, 2023

View reviewed changes

Include/pyhash.h Outdated Show resolved Hide resolved

vstinner reviewed Nov 16, 2023

View reviewed changes

Misc/NEWS.d/next/C API/2023-11-15-09-24-51.gh-issue-111418.FYYetY.rst Outdated Show resolved Hide resolved

Doc/c-api/hash.rst Outdated Show resolved Hide resolved

skirpichev and others added 3 commits November 16, 2023 03:33

Update Misc/NEWS.d/next/C API/2023-11-15-09-24-51.gh-issue-111418.FYY…

00bc891

…etY.rst Co-authored-by: Victor Stinner <vstinner@python.org>

address review: versionadded

6aaa610

Move to Include/cpython/pyhash.h

a8dec64

skirpichev changed the title ~~gh-111389: restoring _PyHASH_INF/BITS/MODULUS/IMAG macroses as public~~ gh-111389: expose _PyHASH_INF/BITS/MODULUS/IMAG macros as public Nov 16, 2023

skirpichev requested a review from vstinner November 16, 2023 00:43

docs nitpick

d3a61ce

Was rendered: "See also the PyTypeObject.tp_hash member and Hashing of numeric types for more details about hashing of numeric types."

bedevere-app bot added awaiting change review and removed awaiting changes labels Mar 4, 2024

vstinner reviewed Mar 8, 2024

View reviewed changes

Include/cpython/pyhash.h Outdated Show resolved Hide resolved

Doc/c-api/hash.rst Outdated Show resolved Hide resolved

Doc/c-api/hash.rst Outdated Show resolved Hide resolved

skirpichev and others added 2 commits March 8, 2024 13:22

Apply suggestions from code review

1f3b623

Co-authored-by: Victor Stinner <vstinner@python.org>

Apply suggestions from review: keep deprecated names as aliases

1dd0fe7

skirpichev requested a review from gpshead as a code owner March 8, 2024 10:30

skirpichev requested a review from vstinner March 8, 2024 10:34

vstinner approved these changes Mar 8, 2024

View reviewed changes

Include/cpython/pyhash.h Show resolved Hide resolved

bedevere-app bot added awaiting merge and removed awaiting change review labels Mar 8, 2024

Add comment

553c663

mdickinson approved these changes Mar 9, 2024

View reviewed changes

vstinner merged commit 1e68c4b into python:main Mar 9, 2024

bedevere-app bot removed the awaiting merge label Mar 9, 2024

skirpichev deleted the restore-pyhash-and-pylongnew branch March 10, 2024 00:11

adorilson pushed a commit to adorilson/cpython that referenced this pull request Mar 25, 2024

pythongh-111389: expose PyHASH_INF/BITS/MODULUS/IMAG macros as public (…

84abeb8

…python#111418) Co-authored-by: Victor Stinner <vstinner@python.org>

diegorusso pushed a commit to diegorusso/cpython that referenced this pull request Apr 17, 2024

pythongh-111389: expose PyHASH_INF/BITS/MODULUS/IMAG macros as public (…

ab7da30

…python#111418) Co-authored-by: Victor Stinner <vstinner@python.org>

Uh oh!

Conversation

skirpichev commented Oct 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vstinner commented Oct 28, 2023

Uh oh!

skirpichev commented Oct 28, 2023

Uh oh!

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

bedevere-app bot commented Oct 30, 2023

Uh oh!

vstinner commented Oct 30, 2023

Uh oh!

ngoldbaum commented Oct 30, 2023

Uh oh!

skirpichev commented Oct 31, 2023

Uh oh!

skirpichev commented Nov 3, 2023

Uh oh!

skirpichev commented Nov 3, 2023

Uh oh!

skirpichev commented Nov 3, 2023

Uh oh!

vstinner commented Nov 4, 2023

Uh oh!

skirpichev commented Nov 5, 2023

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

skirpichev commented Dec 15, 2023

Uh oh!

skirpichev commented Dec 21, 2023

Uh oh!

skirpichev commented Mar 4, 2024

Uh oh!

bedevere-app bot commented Mar 4, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mdickinson left a comment

Choose a reason for hiding this comment

Uh oh!

vstinner commented Mar 9, 2024

Uh oh!

skirpichev commented Mar 10, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

skirpichev commented Oct 28, 2023 •

edited

Loading