Opened 12 years ago

Closed 12 years ago

#9893 closed enhancement (fixed)

Make PARI *not* catch signals

Reported by: jdemeyer Owned by: was
Priority: major Milestone: sage-4.6.1
Component: interfaces Keywords: pari signals
Cc: leif, robertwb Merged in: sage-4.6.1.alpha2
Authors: Jeroen Demeyer Reviewers: Robert Bradshaw
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by jdemeyer)

In Sage, there is no reason for the PARI library to catch signals (by default, it catches SIGBUS, SIGFPE, SIGINT, SIGBREAK, SIGPIPE, SIGSEGV).

The attached patch also simplifies PariInstance.__init__() and removes some unused variables.

Parts of an earlier version this patch has been moved to #9678.

Related: #9678 Dependency: #10115

Attachments (1)

9893_pari_init.patch​ (13.7 KB) - added by jdemeyer 12 years ago.

Download all attachments as: .zip

Change History (23)

comment:1 Changed 12 years ago by jdemeyer

  • Authors set to Jeroen Demeyer
  • Component changed from c_lib to interfaces
  • Description modified (diff)
  • Owner changed from tba to was

comment:2 Changed 12 years ago by jdemeyer

  • Description modified (diff)

comment:3 Changed 12 years ago by jdemeyer

  • Status changed from new to needs_review

comment:4 Changed 12 years ago by leif

  • Cc leif added

comment:5 Changed 12 years ago by jdemeyer

  • Status changed from needs_review to needs_work

This seems to influence #10018, so both #9893 and #10018 should probably be handled together.

comment:6 Changed 12 years ago by jdemeyer

  • Description modified (diff)
  • Milestone changed from sage-4.6 to sage-duplicate/invalid/wontfix
  • Status changed from needs_work to needs_review

comment:7 Changed 12 years ago by davidloeffler

  • Authors Jeroen Demeyer deleted
  • Status changed from needs_review to positive_review

Positive review of the proposal to close as duplicate.

comment:8 Changed 12 years ago by mpatel

  • Resolution set to duplicate
  • Status changed from positive_review to closed

I'm closing this now. Feel free to reopen it, if #10018 ultimately does not subsume this.

comment:9 Changed 12 years ago by jdemeyer

  • Authors set to Jeroen Demeyer
  • Description modified (diff)
  • Keywords pari signals added
  • Milestone changed from sage-duplicate/invalid/wontfix to sage-4.6.1
  • Priority changed from minor to major
  • Status changed from closed to needs_work

Reopening this to collect the minimal changes required in PARI to make #9678 work. This patch here will then become a prerequisite for #9678.

comment:10 Changed 12 years ago by jdemeyer

  • Description modified (diff)

comment:11 Changed 12 years ago by jdemeyer

  • Status changed from needs_work to needs_review

All long doctests pass on a 32-bit PPC OS X 10.4 and on a 64-bit Intel Linux. This patch was first included in #10018, then in #9678 and now I have separated it again in order to make #9678 not too much of a patch bomb. The current version of the patch is pretty much the same as the old version of 5 weeks ago. The patch makes sense by itself, but of course it is partially motivated by #9678. Remark also that currently all tests in sage/libs/pari pass with #9678, #10061 and #10030 applied.

comment:12 Changed 12 years ago by robertwb

  • Status changed from needs_review to needs_work

In general I'm a fan of using try...finally to handle sig_on/sig_off where there are many exits to a function, especially if exceptions are involved. (It can also alleviate the use of temporary variables to return something, but that's a matter of taste.)

Speed is important for new_gen_from_mpz_t, which is why we don't set up signals for "small" values. This behavior should be preserved.

Changed 12 years ago by jdemeyer

comment:13 Changed 12 years ago by jdemeyer

  • Status changed from needs_work to needs_review

comment:14 Changed 12 years ago by jdemeyer

  • Cc robertwb added
  • Reviewers set to Robert Bradshaw

New patch still needs review....

comment:15 follow-up: Changed 12 years ago by robertwb

  • Status changed from needs_review to positive_review

Looks good to me, though I haven't had time to run all tests yet.

comment:16 in reply to: ↑ 15 Changed 12 years ago by jdemeyer

Replying to robertwb:

Looks good to me

Thanks!

comment:17 Changed 12 years ago by jdemeyer

  • Merged in set to sage-4.6.1.alpha1
  • Resolution changed from duplicate to fixed
  • Status changed from positive_review to closed

comment:18 Changed 12 years ago by jdemeyer

  • Merged in sage-4.6.1.alpha1 deleted
  • Resolution fixed deleted
  • Status changed from closed to new

comment:19 Changed 12 years ago by jdemeyer

  • Status changed from new to needs_review

comment:20 Changed 12 years ago by jdemeyer

  • Status changed from needs_review to positive_review

comment:21 Changed 12 years ago by jdemeyer

I decided not to merge this in sage-4.6.1.alpha1 because I thought it might be the cause for #9163 (it turns out that's not the case).

comment:22 Changed 12 years ago by jdemeyer

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