Opened 10 years ago

Closed 10 years ago

#10115 closed enhancement (fixed)

Change _sig_on to sig_on()

Reported by: jdemeyer 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:

Description (last modified by jdemeyer)

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 jdemeyer 10 years ago.
Patch for the examples/ directory
10115_rename_sig_on.patch (386.9 KB) - added by jdemeyer 10 years ago.
Patch against sage-4.6.alpha3

Download all attachments as: .zip

Change History (12)

comment:1 Changed 10 years ago by jdemeyer

  • Description modified (diff)
  • Milestone changed from sage-4.6.1 to sage-4.6
  • Summary changed from Change _sig_on to _sig_on() to Change _sig_on to sig_on()

Changed 10 years ago by jdemeyer

Patch for the examples/ directory

comment:2 Changed 10 years ago by jdemeyer

  • Status changed from new to needs_review

comment:3 Changed 10 years ago by jdemeyer

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

comment:4 Changed 10 years ago by jdemeyer

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

Patch against sage-4.6.alpha3

comment:5 Changed 10 years ago by jdemeyer

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

comment:6 Changed 10 years ago by malb

  • Reviewers set to Martin Albrecht
  • Status changed from needs_review to positive_review

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

comment:7 Changed 10 years ago by jdemeyer

  • Authors set to Jeroen Demeyer

comment:8 Changed 10 years ago by jdemeyer

  • Milestone changed from sage-4.6 to sage-4.6.1
  • Priority changed from major to blocker

comment:9 Changed 10 years ago by robertwb

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

  • Merged in set to sage-4.6.1.alpha0
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.