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

Fix compiler warning in _mpfr_hash() #453

Merged
merged 1 commit into from
Nov 6, 2023

Conversation

skirpichev
Copy link
Contributor

Also drop some inaccessible code in GMPy_MPC_Hash_Slot()


This is cherry-picked from #452. Old discussion: 324e284#commitcomment-131773663

Also drop some inaccessible code in GMPy_MPC_Hash_Slot()
@codecov-commenter
Copy link

codecov-commenter commented Nov 6, 2023

Codecov Report

Merging #453 (6db2b4f) into master (21ccbf7) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

@@            Coverage Diff             @@
##           master     #453      +/-   ##
==========================================
- Coverage   84.97%   84.96%   -0.01%     
==========================================
  Files          49       49              
  Lines       11738    11736       -2     
  Branches     2206     2204       -2     
==========================================
- Hits         9974     9972       -2     
  Misses       1764     1764              
Files Coverage Δ
src/gmpy2_hash.c 100.00% <100.00%> (ø)

📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!

@skirpichev skirpichev mentioned this pull request Nov 6, 2023
@skirpichev
Copy link
Contributor Author

I think the only issue with using PyBaseObject_Type.tp_hash slot is that we can't guarantee that in future CPython versions it's argument will be interpreted as void *. But I think it's highly unlikely to be changed. We can ask to document this.

@casevh
Copy link
Collaborator

casevh commented Nov 6, 2023

Oops. Sorry about the confusion. I missed that.

Again, sorry about my confusion.

@casevh casevh merged commit e002e2c into aleaxit:master Nov 6, 2023
9 checks passed
@skirpichev skirpichev deleted the mpfr-hashing branch November 6, 2023 06:02
@skirpichev
Copy link
Contributor Author

Oops. Sorry about the confusion. I missed that.

@casevh, actually I didn't expect this PR will be merged immediately. Rather it was factored out to discuss hashing of NaN's.

Here is the old discussion (keep in mind next time it's better to comment code in pr, e.g. "File changed" tab, not individual commits: this will be more transparent to other readers): 324e284#commitcomment-131773663
and my above comment: #453 (comment)

So, using PyBaseObject_Type.tp_hash is an exact equivalent of _Py_HashPointer(). While it's not documented, it's hard to imagine a default implementation of the object type hashing to be different in the interface (i.e. dereference the pointer).

I think we have these options:

  1. Ask CPython devs to document hashing of the object type (like for id() builtin).
  2. Ask them to restore _Py_HashPointer() as a public interface (like Make _Py_HashDouble public again as "unstable" API python/cpython#111545)

(1) looks to be better for me, it's hard to imagine where new public interface could be used. Only for hashing of NaN's: https://github.com/python/cpython/blob/ba8aa1fd3735aa3dd13a36ad8f059a422d25ff37/Doc/library/stdtypes.rst?plain=1#L789-L790

@casevh
Copy link
Collaborator

casevh commented Nov 7, 2023

I agree that option (1) is the best choice and that any changes are unlikely.

@skirpichev
Copy link
Contributor Author

Ok, I've asked this in python/cpython#111389.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants