aleaxit / gmpy

General Multi-Precision arithmetic for Python 2.6+/3+ (GMP, MPIR, MPFR, MPC)
https://gmpy2.readthedocs.io/en/latest/
GNU Lesser General Public License v3.0
506 stars 84 forks source link

Fix compiler warning in _mpfr_hash() #453

Closed skirpichev closed 8 months ago

skirpichev commented 8 months ago

Also drop some inaccessible code in GMPy_MPC_Hash_Slot()


This is cherry-picked from #452. Old discussion: https://github.com/aleaxit/gmpy/commit/324e284037a334bf9523ffcf508efe3a854706c4#commitcomment-131773663

codecov-commenter commented 8 months ago

Codecov Report

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

:exclamation: 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%> (ø)

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

skirpichev commented 8 months ago

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 commented 8 months ago

Oops. Sorry about the confusion. I missed that.

Again, sorry about my confusion.

skirpichev commented 8 months ago

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): https://github.com/aleaxit/gmpy/commit/324e284037a334bf9523ffcf508efe3a854706c4#commitcomment-131773663 and my above comment: https://github.com/aleaxit/gmpy/pull/453#issuecomment-1794117906

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 https://github.com/python/cpython/issues/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 commented 8 months ago

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

skirpichev commented 8 months ago

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