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

Priority:  major  Milestone:  sage8.1 
Component:  python3  Keywords:  
Cc:  chapoton, embray  Merged in:  
Authors:  Frédéric Chapoton  Reviewers:  Jeroen Demeyer 
Report Upstream:  N/A  Work issues:  
Branch:  5f9e545 (Commits, GitHub, GitLab)  Commit:  5f9e545b968525d9b08b9698ba0d589bac7759c8 
Dependencies:  Stopgaps: 
Description
Change History (14)
comment:1 Changed 3 years ago by
 Branch set to public/longint24221
 Commit set to 0fb286dda3a0f88c3336153f7996cba676046bc5
 Dependencies set to #24221
 Status changed from new to needs_review
comment:2 followup: ↓ 4 Changed 3 years ago by
 Status changed from needs_review to needs_work
This change is a bit arbitrary:
@@ 3109,6 +3110,11 @@ cdef class Integer(sage.structure.element.EuclideanDomainElement): cdef Integer z cdef long yy, res + if isinstance(y, long): + # This should be treated basically the same as if y is an Integer + # so go ahead and convert y to an Integer first + y = Integer(y) + # First case: Integer % Integer if type(x) is type(y): if not mpz_sgn((<Integer>y).value):
First of all, I think it makes sense to do this both for x
and for y
. Second, could you optimize the Integer()
call a bit by using PY_NEW
and mpz_set_pylong
instead?
comment:4 in reply to: ↑ 2 ; followup: ↓ 7 Changed 3 years ago by
Replying to jdemeyer:
This change is a bit arbitrary:
@@ 3109,6 +3110,11 @@ cdef class Integer(sage.structure.element.EuclideanDomainElement): cdef Integer z cdef long yy, res + if isinstance(y, long): + # This should be treated basically the same as if y is an Integer + # so go ahead and convert y to an Integer first + y = Integer(y) + # First case: Integer % Integer if type(x) is type(y): if not mpz_sgn((<Integer>y).value):First of all, I think it makes sense to do this both for
x
and fory
. Second, could you optimize theInteger()
call a bit by usingPY_NEW
andmpz_set_pylong
instead?
I could be missing some subtlety here but why would you do "this" for x
? This is in Integer.__mod__
so x
would never be a long
.
comment:5 Changed 3 years ago by
 Commit changed from 0fb286dda3a0f88c3336153f7996cba676046bc5 to 94127f907492efe915e6231042384d54d9d51a75
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
94127f9  Check "long" before "int"

comment:6 followup: ↓ 10 Changed 3 years ago by
 Status changed from needs_work to needs_review
I incorporated Jeroen's other suggestion and squashed. Awaiting clarification on the first comment.
comment:7 in reply to: ↑ 4 Changed 3 years ago by
Replying to embray:
I could be missing some subtlety here but why would you do "this" for
x
? This is inInteger.__mod__
sox
would never be along
.
This is a difference between the Python language and the C API.
Python has __mod__
and __rmod__
, the C API has tp_as_number>nb_remainder
where it's not guaranteed that the first argument is of the expected type.
Cython interfaces the C API but with a syntax similar to Python. Most of time, this is useful, but it does cause some confusion. This is probably also the reason why the arguments are called (x, y)
instead of (self, other)
.
comment:8 Changed 3 years ago by
Ah, I wondered if something like that might be the case but I wasn't sure. Okay, I'll clean this up a bit more.
comment:9 Changed 3 years ago by
 Dependencies #24221 deleted
 Status changed from needs_review to needs_work
I'll split off the __mod__
changes.
comment:10 in reply to: ↑ 6 Changed 3 years ago by
comment:11 Changed 3 years ago by
 Commit changed from 94127f907492efe915e6231042384d54d9d51a75 to 5f9e545b968525d9b08b9698ba0d589bac7759c8
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
5f9e545  Check "long" before "int"

comment:12 Changed 3 years ago by
 Reviewers set to Jeroen Demeyer
 Status changed from needs_work to positive_review
comment:13 Changed 3 years ago by
I'll redo Integer.__mod__
on #24293.
comment:14 Changed 3 years ago by
 Branch changed from public/longint24221 to 5f9e545b968525d9b08b9698ba0d589bac7759c8
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
Check "long" before "int"
trac 24221 Eric's suggestions of additional changes