Opened 9 years ago

Closed 8 years ago

#10061 closed defect (fixed)

Fix sig_on/sig_off bugs

Reported by: jdemeyer Owned by: tba
Priority: major Milestone: sage-4.7
Component: c_lib Keywords: interrupt
Cc: vbraun Merged in: sage-4.7.alpha1
Authors: Jeroen Demeyer Reviewers: Volker Braun
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by jdemeyer)

Thanks to #10030, we can check for invalid uses of sig_on()/sig_off(). This ticket will collect those bugs (the bugs do not depend on #9678 or #10030, they were just found using those tickets).

Patch chain: #9678, #10061, #10258, #10030, #10018, #9640

Attachments (2)

10061_fix_sig_on_sig_off.patch (45.7 KB) - added by jdemeyer 8 years ago.
10061_newforms.patch (2.0 KB) - added by jdemeyer 8 years ago.
Additional patch

Download all attachments as: .zip

Change History (19)

comment:1 Changed 9 years ago by jdemeyer

  • Description modified (diff)
  • Keywords interrupt added
  • Milestone changed from sage-4.6 to sage-4.6.1
  • Summary changed from Fix _sig_on/_sig_off bugs to Fix sig_on/sig_off bugs

comment:2 Changed 9 years ago by jdemeyer

  • Description modified (diff)

comment:3 Changed 9 years ago by jdemeyer

  • Authors set to Jeroen Demeyer
  • Status changed from new to needs_work

comment:4 Changed 9 years ago by jdemeyer

  • Description modified (diff)

comment:5 Changed 9 years ago by jdemeyer

  • Description modified (diff)

comment:6 Changed 8 years ago by jdemeyer

  • Description modified (diff)

comment:7 Changed 8 years ago by jdemeyer

  • Status changed from needs_work to needs_review

comment:8 Changed 8 years ago by vbraun

  • Status changed from needs_review to positive_review

Looks good to me.

We (i.e. you) should document somewhere very clearly the use of

   sig_on()
   c_function()
   sig_off()

vs.

   try:
      sig_on()
      cython_function()
   finally:
      sig_off()

In any case, positive review to this ticket.

comment:9 Changed 8 years ago by jdemeyer

  • Reviewers set to Volker Braun

There is already some documentation at #10109, but you are right that it should be expanded.

Changed 8 years ago by jdemeyer

comment:10 Changed 8 years ago by jdemeyer

Rebased patch

comment:11 follow-up: Changed 8 years ago by vbraun

What did you rebase it against? Fails to apply on sage-4.6.2.alpha3

comment:12 in reply to: ↑ 11 Changed 8 years ago by jdemeyer

Replying to vbraun:

What did you rebase it against? Fails to apply on sage-4.6.2.alpha3

The future sage-4.6.2.alpha4 :-)

For this patch, that probably means sage-4.6.2.alpha3 + #10028.

comment:13 Changed 8 years ago by jdemeyer

  • Status changed from positive_review to needs_work

comment:14 Changed 8 years ago by jdemeyer

  • Cc vbraun added
  • Status changed from needs_work to needs_review

Additional patch needs review.

comment:15 Changed 8 years ago by jdemeyer

  • Milestone changed from sage-4.6.2 to sage-4.7

Changed 8 years ago by jdemeyer

Additional patch

comment:16 Changed 8 years ago by vbraun

  • Status changed from needs_review to positive_review

The additional patch looks good to me, too.

comment:17 Changed 8 years ago by jdemeyer

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