Opened 3 years ago
Closed 3 years ago
#24225 closed enhancement (fixed)
py3: check for long before check for int in some pyx files
Reported by:  chapoton  Owned by:  

Priority:  major  Milestone:  sage8.1 
Component:  python3  Keywords:  
Cc:  embray, jdemeyer, tscrim, fbissey  Merged in:  
Authors:  Frédéric Chapoton  Reviewers:  Jeroen Demeyer 
Report Upstream:  N/A  Work issues:  
Branch:  dd3d657 (Commits, GitHub, GitLab)  Commit:  dd3d657688439a87c5d3fbc1b6a308a541383b03 
Dependencies:  Stopgaps: 
Description
as in #24221
Change History (17)
comment:1 Changed 3 years ago by
 Branch set to u/chapoton/24225
 Status changed from new to needs_review
comment:2 Changed 3 years ago by
 Commit set to 542989719c96c40f91eb68f67a276c52b8ce7a80
comment:3 Changed 3 years ago by
Syntax error in src/sage/libs/mpmath/ext_impl.pyx
comment:4 Changed 3 years ago by
The int()
call in plural.pyx
requires further thought. Can you revert that for now?
comment:5 Changed 3 years ago by
 Status changed from needs_review to needs_work
comment:6 Changed 3 years ago by
 Commit changed from 542989719c96c40f91eb68f67a276c52b8ce7a80 to dd3d657688439a87c5d3fbc1b6a308a541383b03
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
dd3d657  py3: check for long before check for int in some pyx files

comment:8 Changed 3 years ago by
 Reviewers set to Jeroen Demeyer
Good for me if the (Python 2) patchbot passes.
comment:9 Changed 3 years ago by
Ah, I just fixed a bunch of these myself :) Glad I don't have to make a ticket for it now.
I might add a comment, at least a few of these cases, making it clear that the isinstance(..., int)
case should never run on Python 3, since this is somewhat nonobvious, at least to me.
comment:10 Changed 3 years ago by
Spoke too soon. Here's a couple more if you want to add them to this ticket; otherwise I can open a new one...

src/sage/rings/integer.pyx
diff git a/src/sage/rings/integer.pyx b/src/sage/rings/integer.pyx index 7e86b95..ff186cc 100644
a b cdef class Integer(sage.structure.element.EuclideanDomainEleme 926 929 c = mpz_cmp((<Integer>left).value, (<Integer>right).value) 927 930 elif isinstance(right, Rational): 928 931 c = mpq_cmp_z((<Rational>right).value, (<Integer>left).value) 929 elif isinstance(right, int):930 c = mpz_cmp_si((<Integer>left).value, PyInt_AS_LONG(right))931 932 elif isinstance(right, long): 932 933 mpz_init(mpz_tmp) 933 934 mpz_set_pylong(mpz_tmp, right) 934 935 c = mpz_cmp((<Integer>left).value, mpz_tmp) 935 936 mpz_clear(mpz_tmp) 937 elif isinstance(right, int): 938 # This case should only occur on Python 2 939 c = mpz_cmp_si((<Integer>left).value, PyInt_AS_LONG(right)) 936 940 elif isinstance(right, float): 937 941 d = right 938 942 if isnan(d): … … cdef class Integer(sage.structure.element.EuclideanDomainElem 3115 3119 cdef Integer z 3116 3120 cdef long yy, res 3117 3121 3122 if isinstance(y, long): 3123 # This should be treated basically the same as if y is an Integer 3124 # so go ahead and convert y to an Integer first 3125 y = Integer(y) 3126 3118 3127 # First case: Integer % Integer 3119 3128 if type(x) is type(y): 3120 3129 if not mpz_sgn((<Integer>y).value):
Edit: Originally a few of these diffs were the same as in #24221, but I think the ones here now are still new...
comment:12 Changed 3 years ago by
maybe we should rather add them to #24221 to minimize conflicts
comment:13 followup: ↓ 15 Changed 3 years ago by
I have added your additional changes to #24221, that is back to "needs review"
comment:14 Changed 3 years ago by
green bot here
comment:15 in reply to: ↑ 13 Changed 3 years ago by
comment:16 Changed 3 years ago by
 Status changed from needs_review to positive_review
comment:17 Changed 3 years ago by
 Branch changed from u/chapoton/24225 to dd3d657688439a87c5d3fbc1b6a308a541383b03
 Resolution set to fixed
 Status changed from positive_review to closed
Branch pushed to git repo; I updated commit sha1. New commits:
py3: check for long before check for int in some pyx files