Opened 7 years ago
Closed 7 years ago
#13746 closed defect (fixed)
Do not delete a borrowed reference to reduction strategies in pbori
| Reported by: | jpflori | Owned by: | tbd |
|---|---|---|---|
| Priority: | blocker | Milestone: | sage-5.5 |
| Component: | memleak | Keywords: | segfault |
| Cc: | SimonKing, AlexanderDreyer | 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: |
Attachments (1)
Change History (12)
Changed 7 years ago by
comment:1 Changed 7 years ago by
- Component changed from PLEASE CHANGE to memleak
- Status changed from new to needs_review
comment:2 follow-up: ↓ 3 Changed 7 years ago by
comment:3 in reply to: ↑ 2 Changed 7 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 7 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 7 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 7 years ago by
- Cc AlexanderDreyer added
comment:7 Changed 7 years ago by
- Milestone changed from sage-5.6 to sage-5.5
- Priority changed from critical to blocker
comment:8 follow-up: ↓ 9 Changed 7 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 in reply to: ↑ 8 Changed 7 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 7 years ago by
- Reviewers set to Alexander Dreyer
- Status changed from needs_review to positive_review
Looks good!
comment:11 Changed 7 years ago by
- Merged in set to sage-5.5.rc1
- Resolution set to fixed
- Status changed from positive_review to closed
Patch from #12313 with only changes concerning trac ticket numbers