Opened 13 years ago

Closed 13 years ago

#6590 closed defect (fixed)

[with patch, positive review] Cython __new__ should be __cinit__

Reported by: Robert Bradshaw Owned by: tbd
Priority: major Milestone: sage-4.1.2
Component: build Keywords:
Cc: Merged in: Sage 4.1.2.alpha0
Authors: Robert Bradshaw Reviewers: Alex Ghitza
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description

This changed a while back, but as long as the old form is in the library we won't be able to really implement a (Python-style) __new__ and also people will keep using it in new code by analogy.

Attachments (2)

6590-cinit.patch (42.6 KB) - added by Robert Bradshaw 13 years ago.
6590-cinit_rebased.patch (43.4 KB) - added by Alex Ghitza 13 years ago.
rebased against sage-4.1.1, apply instead of previous patch

Download all attachments as: .zip

Change History (6)

Changed 13 years ago by Robert Bradshaw

Attachment: 6590-cinit.patch added

comment:1 Changed 13 years ago by Jason Grout

How come the some of the new cinit functions have different signatures from the corresponding init functions? I thought the signatures should be the same, or at least the cinit should have a *args or kwds to accept the arguments passed to init

In particular, I refer to sage/libs/ntl/ntl_mat_GF2.pyx, sage/libs/ntl/ntl_mat_ZZ.pyx, sage/matrix/matrix_integer_2x2.pyx, etc.

comment:2 Changed 13 years ago by Robert Bradshaw

If no init parameters are not needed by cinit they can simply be omitted. This saves on argument-parsing code, especially as kwds needs to construct an empty dictionary each time. Also note that cinit is called on PY_NEW, so the savings here is really nice. (Essentially, __cinit__(self) is implicitly __cinit__(self, *args, **kwds) where *args and kwds are not parsed because they're not needed.

Changed 13 years ago by Alex Ghitza

Attachment: 6590-cinit_rebased.patch added

rebased against sage-4.1.1, apply instead of previous patch

comment:3 Changed 13 years ago by Alex Ghitza

Authors: Robert Bradshaw
Reviewers: Alex Ghitza
Summary: [with patch, needs review] Cython __new__ should be __cinit__[with patch, positive review] Cython __new__ should be __cinit__

Positive review. I had to rebase it against sage-4.1.1 since it didn't apply cleanly.

comment:4 Changed 13 years ago by Minh Van Nguyen

Merged in: Sage 4.1.2.alpha0
Resolution: fixed
Status: newclosed

Merged 6590-cinit_rebased.patch.

Note: See TracTickets for help on using tickets.