Opened 10 years ago
Closed 10 years ago
#13746 closed defect (fixed)
Do not delete a borrowed reference to reduction strategies in pbori
| Reported by: | Jean-Pierre Flori | Owned by: | tbd |
|---|---|---|---|
| Priority: | blocker | Milestone: | sage-5.5 |
| Component: | memleak | Keywords: | segfault |
| Cc: | Simon King, Alexander Dreyer | Merged in: | sage-5.5.rc1 |
| Authors: | Jean-Pierre Flori, Simon King | Reviewers: | Alexander Dreyer |
| Report Upstream: | N/A | Work issues: | |
| Branch: | Commit: | ||
| Dependencies: | Stopgaps: |
Status badges
Attachments (1)
Change History (12)
Changed 10 years ago by
| Attachment: | trac_12313_quit_sage.patch added |
|---|
comment:1 Changed 10 years ago by
| Component: | PLEASE CHANGE → memleak |
|---|---|
| Status: | new → needs_review |
comment:2 follow-up: 3 Changed 10 years ago by
comment:3 Changed 10 years ago by
Replying to SimonKing:
OK, you took the patch from #12313 and removed the bits that now belong to #13741, right?
No, the two patches were orthogonal, they touched different part of quit_sage. I'm not completely sure the additional garbage collection we introduce here is really needed, but it cannot hurt for sure (or it can reveal nasty bugs :) which is good).
I don't recall who wrote most of the code in the patch - in other words: Who can be reviewer for it?
I think the patch was already positively reviewed during the process of reviewing #12313, so you or Nils could set this ticket to positive review, I just don't feel doing it as I've opened the ticket.
comment:4 Changed 10 years ago by
And so this is exactly the same patch as in #12313, except that:
- I've replace 12313 by 13746 in the commit message
- I've replace 12313 by 12313 and 13746 in some comment in the code redirecting the reader to Sage's trac
comment:5 Changed 10 years ago by
Replying to jpflori:
Replying to SimonKing:
OK, you took the patch from #12313 and removed the bits that now belong to #13741, right?
No, the two patches were orthogonal,
Sorry, you are totally right! What I moved to #13471 came from #12215 or so...
I'm not completely sure the additional garbage collection we introduce here is really needed, but it cannot hurt for sure (or it can reveal nasty bugs :) which is good).
Indeed!
I think the patch was already positively reviewed during the process of reviewing #12313, so you or Nils could set this ticket to positive review, I just don't feel doing it as I've opened the ticket.
Not before running a full test...
comment:6 Changed 10 years ago by
| Cc: | Alexander Dreyer added |
|---|
comment:7 Changed 10 years ago by
| Milestone: | sage-5.6 → sage-5.5 |
|---|---|
| Priority: | critical → blocker |
comment:8 follow-up: 9 Changed 10 years ago by
I could probably review it, since I've already discussed it previously. The patch looks sane, yet I don't understand, why it fails on the patchbot. (Is this related to something else?)
comment:9 Changed 10 years ago by
Replying to AlexanderDreyer:
I could probably review it, since I've already discussed it previously. The patch looks sane, yet I don't understand, why it fails on the patchbot. (Is this related to something else?)
My guess is that the error is unrelated (it seems to come from a killed Gap session). To be on the safe side, I've just kicked the patchbot, so that it should now repeat the tests.
comment:10 Changed 10 years ago by
| Reviewers: | → Alexander Dreyer |
|---|---|
| Status: | needs_review → positive_review |
Looks good!
comment:11 Changed 10 years ago by
| Merged in: | → sage-5.5.rc1 |
|---|---|
| Resolution: | → fixed |
| Status: | positive_review → closed |
Patch from #12313 with only changes concerning trac ticket numbers