Opened 8 years ago

Closed 8 years ago

#18772 closed enhancement (fixed)

Completely remove in-place operations

Reported by: jdemeyer Owned by:
Priority: minor Milestone: sage-6.8
Component: coercion Keywords:
Cc: ncohen Merged in:
Authors: Jeroen Demeyer Reviewers: François Bissey
Report Upstream: N/A Work issues:
Branch: 1857b02 (Commits, GitHub, GitLab) Commit: 1857b025948ecbdf1093952832096e986deff969
Dependencies: Stopgaps:

Status badges

Description (last modified by jdemeyer)

Completely remove the code added by #624 and disabled in #1038.

See also ​https://groups.google.com/forum/#!searchin/sage-devel/in$20place/sage-devel/X3j656VKhuc/nCuW_2dcp-IJ

Change History (9)

comment:1 Changed 8 years ago by ncohen

Cc: ncohen added

(being curious, though I have no idea what this ticket is about)

comment:2 Changed 8 years ago by jdemeyer

Description: modified (diff)

comment:3 Changed 8 years ago by jdemeyer

Branch: → u/jdemeyer/completely_remove_in_place_operations

comment:4 Changed 8 years ago by jdemeyer

Commit: → 1857b025948ecbdf1093952832096e986deff969
Status: new → needs_review

New commits:

​1857b02Completely remove in-place operations

comment:5 Changed 8 years ago by jdemeyer

Description: modified (diff)

comment:6 Changed 8 years ago by fbissey

Reviewers: → François Bissey
Status: needs_review → positive_review

Looks good to me.

comment:7 Changed 8 years ago by nbruin

Status: positive_review → needs_info

I don't mind if this ticket gets put straight back to positive, but perhaps you can check:

On #624 there is mention of a special option for Cython that is required. If we're ripping the whole in-place stuff out, we can probably also get rid of this Cython option. Since that should save an incref/decref it might lead to slightly faster code. Might be worth looking at?

hm, the relevant option seems to be --incref-local-binop, which by the looks of it was already disabled on #3331. So we're probably good. Can someone confirm?

Last edited 8 years ago by nbruin (previous) (diff)

comment:8 in reply to:  7 Changed 8 years ago by jdemeyer

Status: needs_info → positive_review

Replying to nbruin:

hm, the relevant option seems to be --incref-local-binop, which by the looks of it was already disabled on #3331.

Grepping both the Cython and Sage source code for incref-local-binop yielded no matches, so it looks good indeed.

comment:9 Changed 8 years ago by vbraun

Branch: u/jdemeyer/completely_remove_in_place_operations → 1857b025948ecbdf1093952832096e986deff969
Resolution: → fixed
Status: positive_review → closed
Note: See TracTickets for help on using tickets.