Opened 5 years ago

Closed 5 years ago

py3: check for long before check for int in some pyx files (part 2)

Reported by: Owned by: jdemeyer major sage-8.1 python3 chapoton, embray Frédéric Chapoton Jeroen Demeyer N/A 5f9e545 5f9e545b968525d9b08b9698ba0d589bac7759c8

comment:1 Changed 5 years ago by jdemeyer

• Branch set to public/longint24221
• Commit set to 0fb286dda3a0f88c3336153f7996cba676046bc5
• Dependencies set to #24221
• Status changed from new to needs_review

New commits:

 ​190a90c `Check "long" before "int"` ​0fb286d `trac 24221 Eric's suggestions of additional changes`

comment:2 follow-up: ↓ 4 Changed 5 years ago by jdemeyer

• 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:3 Changed 5 years ago by chapoton

Erik, could you handle Jeroen's queries ?

comment:4 in reply to: ↑ 2 ; follow-up: ↓ 7 Changed 5 years ago by embray

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?

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 5 years ago by git

• 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 follow-up: ↓ 10 Changed 5 years ago by embray

• 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 5 years ago by jdemeyer

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`.

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 5 years ago by embray

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 5 years ago by jdemeyer

• 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 5 years ago by jdemeyer

I incorporated Jeroen's other suggestion and squashed.

Don't squash! The first commit came from the dependency #24221. Let me try to fix this mess...

comment:11 Changed 5 years ago by git

• 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 5 years ago by jdemeyer

• Reviewers set to Jeroen Demeyer
• Status changed from needs_work to positive_review

comment:13 Changed 5 years ago by jdemeyer

I'll redo `Integer.__mod__` on #24293.

comment:14 Changed 5 years ago by vbraun

• Branch changed from public/longint24221 to 5f9e545b968525d9b08b9698ba0d589bac7759c8
• Resolution set to fixed
• Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.