Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use correct method to delete resource #783

Merged
merged 2 commits into from
Dec 11, 2024
+3 −3
Merged

Use correct method to delete resource #783

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
72cbc51
Use correct method to delete resource
tobiolo Dec 11, 2024
21d3727
Do not destroy resource previously allocated
tobiolo Dec 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/cell.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ struct Cell {
}

void Clear() {
grid.release();
grid.reset();
text.t.Clear();
text.image = nullptr;
Reset();
Expand Down
4 changes: 2 additions & 2 deletions src/grid.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -513,7 +513,7 @@ struct Grid {
}
if (!cell->parent) return; // FIXME: deletion of root cell, what would be better?
s = cell->parent->grid->FindCell(cell);
cell->grid.release();
cell->grid.reset();
}

void InsertCells(int dx, int dy, int nxs, int nys, Cell *nc = nullptr) {
Expand Down Expand Up @@ -668,7 +668,7 @@ struct Grid {
}

void MergeWithParent(Grid *p, Selection &s, Document *doc) {
cell->grid = nullptr;
cell->grid.release();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can merge this commit and think about the above feedback for a separate commit, just to stop the crashing.

Copy link
Collaborator Author

@tobiolo tobiolo Dec 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, but in that case maybe some more comments in the code locations where this happens would be good.

foreachcell(c) {
if (x + s.x >= p->xs) p->InsertCells(p->xs, -1, 1, 0);
if (y + s.y >= p->ys) p->InsertCells(-1, p->ys, 0, 1);
Expand Down
Loading