Use correct method to delete resource #783
Conversation
There was a problem hiding this comment.
This has been very wild.
Kudos to AddressSanitizer.
Before this PR, Cell::Paste(...) got a new Grid it owns, but Grid::MergeWithParent(...) deallocated that Grid because of cell->grid = nullptr triggering the Deleter because it is a unique_ptr. But we need the cells in that grid to populate the parent grid for the merge. Before this fix, TreeSheets crashes when copy & paste a grid (e.g. two cells). With release(), we detach the Grid from Cell without deleting it.
| @@ -668,7 +668,7 @@ struct Grid { | |||
| } | |||
|
|
|||
| void MergeWithParent(Grid *p, Selection &s, Document *doc) { | |||
| cell->grid = nullptr; | |||
| cell->grid.release(); | |||
There was a problem hiding this comment.
This is still not so great code, because you remove the pointer from the unique_ptr, and then "throw it away". We know there is an owner elsewhere, but that is not obvious from this code. So at the very least there should be a comment indicating who is the new owner and why. Ideally, it be even better if the code was structured such that release was used at the very moment the new owner takes the pointer, but only if that doesn't otherwise complicate the code.
There was a problem hiding this comment.
You can merge this commit and think about the above feedback for a separate commit, just to stop the crashing.
There was a problem hiding this comment.
Ok I will think about your feedback. The issue in my opinion is that the pasted grid is cloned into the grid owned by the cell and that is it, or the same happens but the grid is directly dissolved because it is merged. So there is a fifty-fifty chance that the owner (Cell/or unique_ptr in Cell responsible for Grid) keeps the grid or loses it directly after gaining it. With MergeWithParent and the circular reference (cell->grid) the Grid detaches itself from its owner so the Grid has no owner anymore.
No description provided.