Opened 12 years ago

Closed 12 years ago

#10115 closed enhancement (fixed)

Change _sig_on to sig_on()

Reported by: Jeroen Demeyer Owned by: tba
Priority: blocker Milestone: sage-4.6.1
Component: c_lib Keywords: _sig_on _sig_off
Cc: Merged in: sage-4.6.1.alpha0
Authors: Jeroen Demeyer Reviewers: Martin Albrecht
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by Jeroen Demeyer)

Right now, _sig_on is a macro. However, in the light of future changes at #9678, it would be better to make it have function syntax and write _sig_on() instead. The same for _sig_off. While we're at it, why not drop the leading underscore and make it sig_on()? The sig_on macro/function isn't really "private", it's used all over the place and it should be used by users writing their own Cython code.

So in this ticket, I would simpy change the syntax from _sig_on to sig_on() without changing what it does.

This will prepare the way for #9678 when sig_on() will actually become an inline function (there are several reasons why this would be better).

Attachments (2)

10115_rename_sig_on_examples.patch (1.7 KB) - added by Jeroen Demeyer 12 years ago.
Patch for the examples/ directory
10115_rename_sig_on.patch (386.9 KB) - added by Jeroen Demeyer 12 years ago.
Patch against sage-4.6.alpha3

Download all attachments as: .zip

Change History (12)

comment:1 Changed 12 years ago by Jeroen Demeyer

Description: modified (diff)
Milestone: sage-4.6.1sage-4.6
Summary: Change _sig_on to _sig_on()Change _sig_on to sig_on()

Changed 12 years ago by Jeroen Demeyer

Patch for the examples/ directory

comment:2 Changed 12 years ago by Jeroen Demeyer

Status: newneeds_review

comment:3 Changed 12 years ago by Jeroen Demeyer

A very minor update of the patch, the only change is in whitespace.

comment:4 Changed 12 years ago by Jeroen Demeyer

To further justify the "innocence" of this patch, I checked the md5sums of all .so files generated without and with this patch and the only .so file with a different md5sum is c_lib/libcsage.so (I guess only because a string changed). So the code which is generated is literally the same with or without this patch.

Changed 12 years ago by Jeroen Demeyer

Attachment: 10115_rename_sig_on.patch added

Patch against sage-4.6.alpha3

comment:5 Changed 12 years ago by Jeroen Demeyer

New version re-adds _sig_on, _sig_str(s) and _sig_off for backwards compatibility.

comment:6 Changed 12 years ago by Martin Albrecht

Reviewers: Martin Albrecht
Status: needs_reviewpositive_review

I read the patch, it does what it advertises. It also applies cleanly to 4.6.alpha3 and long doctests.

comment:7 Changed 12 years ago by Jeroen Demeyer

Authors: Jeroen Demeyer

comment:8 Changed 12 years ago by Jeroen Demeyer

Milestone: sage-4.6sage-4.6.1
Priority: majorblocker

comment:9 Changed 12 years ago by Robert Bradshaw

I've analysed the patch as well, looks fine by me.

Really, this (or something similar) should be offered as part of Cython itself.

comment:10 Changed 12 years ago by Jeroen Demeyer

Merged in: sage-4.6.1.alpha0
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.