Opened 7 years ago

Closed 6 years ago

Last modified 6 years ago

#20740 closed defect (fixed)

Drop return type from arithmetic methods in coercion model

Reported by: Jeroen Demeyer Owned by:
Priority: major Milestone: sage-7.3
Component: coercion Keywords: days74
Cc: Vincent Delecroix Merged in:
Authors: Jeroen Demeyer Reviewers: Marc Mezzarobba
Report Upstream: N/A Work issues:
Branch: 56776e2 (Commits, GitHub, GitLab) Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by Jeroen Demeyer)

Replace

cpdef ModuleElement _add_(left, ModuleElement right)

by

cpdef _add_(left, ModuleElement right)

and similar. The return type serves no purpose:

  1. if you really do need any information of the return type, then something like ModuleElement is too generic anyway.
  2. it forces Cython to add some checks that the returned value is of the correct type, so it might actually slow down things.

Also remove the superfluous declarations of these methods in other .pxd files.

Change History (12)

comment:1 Changed 7 years ago by Jeroen Demeyer

Description: modified (diff)

comment:2 Changed 7 years ago by Jeroen Demeyer

Description: modified (diff)

comment:3 Changed 7 years ago by Jeroen Demeyer

(edit: wrong ticket)

Last edited 6 years ago by Jeroen Demeyer (previous) (diff)

comment:4 Changed 6 years ago by Jeroen Demeyer

Branch: u/jdemeyer/drop_return_type

comment:5 Changed 6 years ago by Jeroen Demeyer

Commit: 56776e26371ba92fcab8aaa33500835b78ad64fa
Status: newneeds_review

New commits:

56776e2Drop return type from single-underscore arithmetic methods

comment:6 Changed 6 years ago by Jeroen Demeyer

Keywords: days74 added

comment:7 Changed 6 years ago by Marc Mezzarobba

Status: needs_reviewpositive_review

Sounds convincing, though I would have preferred someone with stronger Cython-fu than me to review the ticket :-). This does make a few generated C files a couple hundred lines smaller, but I didn't notice any significant performance change (either way).

comment:8 Changed 6 years ago by Marc Mezzarobba

Reviewers: Marc Mezzarobba

comment:9 in reply to:  7 Changed 6 years ago by Jeroen Demeyer

Replying to mmezzarobba:

but I didn't notice any significant performance change (either way).

I'm not surprised. The overhead of these checks should be small compared to the actual operation.

Thanks for the review of this and related tickets.

comment:10 Changed 6 years ago by Jeroen Demeyer

Summary: Drop return type from single-underscore arithmetic methodsDrop return type from arithmetic methods in coercion model

comment:11 Changed 6 years ago by Volker Braun

Branch: u/jdemeyer/drop_return_type56776e26371ba92fcab8aaa33500835b78ad64fa
Resolution: fixed
Status: positive_reviewclosed

comment:12 Changed 6 years ago by William Stein

Commit: 56776e26371ba92fcab8aaa33500835b78ad64fa

test

Note: See TracTickets for help on using tickets.