GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-111389: expose _PyHASH_INF/BITS/MODULUS/IMAG macros as public by skirpichev · Pull Request #111418 · python/cpython

Conversation

@skirpichev

@skirpichev skirpichev commented Oct 28, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member
  • Issue: Consider restoring _PyHASH_INF/BITS/MODULUS/IMAG as public #111389

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@skirpichev
skirpichev requested a review from tiran as a code owner October 28, 2023 02:24
@bedevere-app bedevere-app Bot added the awaiting review label Oct 28, 2023
@bedevere-app bedevere-app Bot mentioned this pull request Oct 28, 2023
Consider restoring _PyHASH_INF/BITS/MODULUS/IMAG as public #111389
Closed
@skirpichev
pythongh-111389: restoring _PyHASH_INF/BITS/MODULUS/IMAG macroses as …
0ce377a
…public

This partially reverts python#107026.
@skirpichev
skirpichev force-pushed the restore-pyhash-and-pylongnew branch from 1edec34 to 0ce377a Compare October 28, 2023 03:52
@vstinner

vstinner commented Oct 28, 2023

Copy link
Copy Markdown
Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@skirpichev

skirpichev commented Oct 28, 2023

Copy link
Copy Markdown
Member Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@skirpichev
Mention public macros in docs
2c6002f
@skirpichev
skirpichev force-pushed the restore-pyhash-and-pylongnew branch from 1f9f2fc to 2c6002f Compare October 29, 2023 04:52
vstinner
vstinner reviewed Oct 30, 2023
View reviewed changes
Comment thread Doc/library/sys.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

skirpichev added 2 commits October 30, 2023 16:29
@skirpichev
Revert "Mention public macros in docs"
5c42cae
This reverts commit 2c6002f.
@skirpichev
Add Doc/c-api/hashing.rst for hash-related C macros
de4e3ae
vstinner
vstinner requested changes Oct 30, 2023
View reviewed changes

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

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.

Please don't document private APIs.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@bedevere-app bedevere-app Bot added awaiting changes and removed awaiting review labels Oct 30, 2023
@bedevere-app

bedevere-app Bot commented Oct 30, 2023

Copy link
Copy Markdown

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@befeleme 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
@vstinner

vstinner commented Oct 30, 2023

Copy link
Copy Markdown
Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@ngoldbaum

ngoldbaum commented Oct 30, 2023

Copy link
Copy Markdown
Contributor

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.

seberg reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@skirpichev
Expose macroses as unstable API
1a50a7b
@skirpichev

skirpichev commented Oct 31, 2023

Copy link
Copy Markdown
Member Author

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)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@seberg seberg mentioned this pull request Oct 31, 2023
Make _Py_HashDouble public again as "unstable" API #111545
Closed
@skirpichev
skirpichev requested a review from vstinner November 1, 2023 14:26
@skirpichev

skirpichev commented Nov 3, 2023

Copy link
Copy Markdown
Member Author

I have made the requested changes; please review again

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@skirpichev

skirpichev commented Nov 3, 2023

Copy link
Copy Markdown
Member Author

I didn't expect the Spanish Inquisition

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@skirpichev

skirpichev commented Nov 3, 2023

Copy link
Copy Markdown
Member Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@skirpichev skirpichev mentioned this pull request Nov 3, 2023
Is Bedevere retired? python/bedevere#604
Closed
@vstinner

vstinner commented Nov 4, 2023

Copy link
Copy Markdown
Member

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

skirpichev reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@skirpichev

skirpichev commented Nov 5, 2023

Copy link
Copy Markdown
Member Author

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?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

vstinner
vstinner reviewed Nov 15, 2023
View reviewed changes

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

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.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Include/pyhash.h Resolved

Uh oh!

There was an error while loading. Please reload this page.

vstinner
vstinner reviewed Nov 16, 2023
View reviewed changes
Comment thread Misc/NEWS.d/next/C API/2023-11-15-09-24-51.gh-issue-111418.FYYetY.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Doc/c-api/hash.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

skirpichev and others added 3 commits November 16, 2023 03:33
@skirpichev @vstinner
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 <[email protected]>
@skirpichev
address review: versionadded
6aaa610
@skirpichev
Move to Include/cpython/pyhash.h
a8dec64
@skirpichev 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
skirpichev requested a review from vstinner November 16, 2023 00:43
@skirpichev
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."
@skirpichev

skirpichev commented Dec 15, 2023

Copy link
Copy Markdown
Member Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@skirpichev

skirpichev commented Dec 21, 2023

Copy link
Copy Markdown
Member Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@skirpichev

skirpichev commented Mar 4, 2024

Copy link
Copy Markdown
Member Author

I have made the requested changes; please review again

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

bedevere-app Bot commented Mar 4, 2024

Copy link
Copy Markdown

Thanks for making the requested changes!

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

vstinner
vstinner reviewed Mar 8, 2024
View reviewed changes
Comment thread Include/cpython/pyhash.h Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Doc/c-api/hash.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Doc/c-api/hash.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

skirpichev and others added 2 commits March 8, 2024 13:22
@skirpichev @vstinner
Apply suggestions from code review
1f3b623
Co-authored-by: Victor Stinner <[email protected]>
@skirpichev
Apply suggestions from review: keep deprecated names as aliases
1dd0fe7
@skirpichev
skirpichev requested a review from gpshead as a code owner March 8, 2024 10:30
@skirpichev
skirpichev requested a review from vstinner March 8, 2024 10:34
vstinner
vstinner approved these changes Mar 8, 2024
View reviewed changes

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

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.

LGTM, I just suggest to add a comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Include/cpython/pyhash.h Resolved

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting change review labels Mar 8, 2024
@skirpichev
Add comment
553c663
mdickinson
mdickinson approved these changes Mar 9, 2024
View reviewed changes

@mdickinson mdickinson left a comment

Copy link
Copy Markdown
Member

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.

Looks fine to me.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@vstinner
vstinner merged commit 1e68c4b into python:main Mar 9, 2024
@bedevere-app bedevere-app Bot removed the awaiting merge label Mar 9, 2024
@vstinner

vstinner commented Mar 9, 2024

Copy link
Copy Markdown
Member

Merged, thank you @skirpichev.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

skirpichev commented Mar 10, 2024

Copy link
Copy Markdown
Member Author

Thanks to all for review and patience.

vstinner reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

adorilson pushed a commit to adorilson/cpython that referenced this pull request Mar 25, 2024
@skirpichev @vstinner @adorilson
pythongh-111389: expose PyHASH_INF/BITS/MODULUS/IMAG macros as public (…
84abeb8
…python#111418)

Co-authored-by: Victor Stinner <[email protected]>
diegorusso pushed a commit to diegorusso/cpython that referenced this pull request Apr 17, 2024
@skirpichev @vstinner @diegorusso
pythongh-111389: expose PyHASH_INF/BITS/MODULUS/IMAG macros as public (…
ab7da30
…python#111418)

Co-authored-by: Victor Stinner <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vstinner vstinner vstinner approved these changes
@tiran tiran Awaiting requested review from tiran
@gpshead gpshead Awaiting requested review from gpshead gpshead is a code owner
+1 more reviewer
@mdickinson mdickinson mdickinson approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@skirpichev @vstinner @ngoldbaum @mdickinson
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.