py3: check for long before check for int in some pyx files

[fchapoton]

as in #24221

CC: @embray @jdemeyer @tscrim @kiwifb

Component: python3

Author: Frédéric Chapoton

Branch/Commit: dd3d657

Reviewer: Jeroen Demeyer

Issue created by migration from https://trac.sagemath.org/ticket/24225

[fchapoton]

Branch: u/chapoton/24225

[sagetrac-git]

Commit: 5429897

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

5429897

py3: check for long before check for int in some pyx files

[jdemeyer]

comment:3

Syntax error in src/sage/libs/mpmath/ext_impl.pyx

[jdemeyer]

comment:4

The int() call in plural.pyx requires further thought. Can you revert that for now?

[sagetrac-git]

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

[sagetrac-git]

Changed commit from 5429897 to dd3d657

[fchapoton]

comment:7

Done, thanks.

[jdemeyer]

comment:8

Good for me if the (Python 2) patchbot passes.

[jdemeyer]

Reviewer: Jeroen Demeyer

[embray]

comment:9

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 non-obvious, at least to me.

[embray]

comment:10

Spoke too soon. Here's a couple more if you want to add them to this ticket; otherwise I can open a new one...

diff --git a/src/sage/rings/integer.pyx b/src/sage/rings/integer.pyx
index 7e86b95..ff186cc 100644
--- a/src/sage/rings/integer.pyx
+++ b/src/sage/rings/integer.pyx
@@ -926,13 +929,14 @@ cdef class Integer(sage.structure.element.EuclideanDomainEleme
             c = mpz_cmp((<Integer>left).value, (<Integer>right).value)
         elif isinstance(right, Rational):
             c = -mpq_cmp_z((<Rational>right).value, (<Integer>left).value)
-        elif isinstance(right, int):
-            c = mpz_cmp_si((<Integer>left).value, PyInt_AS_LONG(right))
         elif isinstance(right, long):
             mpz_init(mpz_tmp)
             mpz_set_pylong(mpz_tmp, right)
             c = mpz_cmp((<Integer>left).value, mpz_tmp)
             mpz_clear(mpz_tmp)
+        elif isinstance(right, int):
+            # This case should only occur on Python 2
+            c = mpz_cmp_si((<Integer>left).value, PyInt_AS_LONG(right))
         elif isinstance(right, float):
             d = right
             if isnan(d):
@@ -3115,6 +3119,11 @@ cdef class Integer(sage.structure.element.EuclideanDomainElem
         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):

Edit: Originally a few of these diffs were the same as in #24221, but I think the ones here now are still new...

[fchapoton]

comment:11

Those are done in #24221

oh, I missed your edit..

[fchapoton]

comment:12

maybe we should rather add them to #24221 to minimize conflicts

[fchapoton]

comment:13

I have added your additional changes to #24221, that is back to "needs review"

[fchapoton]

comment:14

green bot here

[jdemeyer]

comment:15

Replying to @fchapoton:

I have added your additional changes to #24221

Moved that to #24227 instead.

[vbraun]

Changed branch from u/chapoton/24225 to dd3d657