Add tree_pos to tsk_tree_t struct #2867
Conversation
duncanMR
force-pushed
the
tree_pos_revision
branch
from
7d5df12 to
25d30fb
Compare
|
@jeromekelleher I see why you made the comment about doxygen in the trees header file: moving |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #2867 +/- ##
===========================================
- Coverage 89.69% 77.02% -12.68%
===========================================
Files 30 30
Lines 30159 30146 -13
Branches 5860 5596 -264
===========================================
- Hits 27052 23220 -3832
- Misses 1778 5538 +3760
- Partials 1329 1388 +59
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 16 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
c/tskit/trees.c
Outdated
| @@ -4520,12 +4520,14 @@ tsk_tree_init(tsk_tree_t *self, const tsk_treeseq_t *tree_sequence, tsk_flags_t | |||
| { | |||
| int ret = TSK_ERR_NO_MEMORY; | |||
| tsk_size_t num_samples, num_nodes, N; | |||
| tsk_tree_position_t tree_pos; | |||
There was a problem hiding this comment.
Don't bother making a separate tree_pos struct here, it won't make any difference. One thing to realise is that
tsk_memset(self, 0, sizeof(tsk_tree_t));
will also zero out the embedded tree_position_t struct.
c/tskit/trees.c
Outdated
| @@ -4565,6 +4568,11 @@ tsk_tree_init(tsk_tree_t *self, const tsk_treeseq_t *tree_sequence, tsk_flags_t | |||
| goto out; | |||
| } | |||
| } | |||
|
|
|||
| ret = tsk_tree_position_init(&tree_pos, tree_sequence, 0); | |||
There was a problem hiding this comment.
| ret = tsk_tree_position_init(&tree_pos, tree_sequence, 0); | |
| ret = tsk_tree_position_init(&self->tree_pos, tree_sequence, 0); |
c/tskit/trees.c
Outdated
| @@ -4613,6 +4621,7 @@ tsk_tree_free(tsk_tree_t *self) | |||
| tsk_safe_free(self->next_sample); | |||
| tsk_safe_free(self->num_children); | |||
| tsk_safe_free(self->edge); | |||
| tsk_safe_free(self->tree_pos); | |||
There was a problem hiding this comment.
This is wrong , and I'm surprised valgrind doesn't catch it. The golden rule is, you only free a pointer than you have previously malloced. The tree_pos here is an embedded struct, so it's not a pointer that can be freed.
You should be calling tsk_tree_position_free(&self->tree_pos); instead.
There was a problem hiding this comment.
Will do: thanks for the tip! At the moment, tsk_tree_position_free doesn't do anything:
int
tsk_tree_position_free(tsk_tree_position_t *TSK_UNUSED(self))
{
return 0;
}
Was this just a placeholder, or is there nothing we need to free?
There was a problem hiding this comment.
It's just a placeholder, better to have it in client code and use it in case we end up doing some mallocing later. Writing good C code depends a lot on following conventions, and it's much easier to just follow a given set of conventions than to think locally about how things should be done.
c/tskit/trees.h
Outdated
| const tsk_treeseq_t *tree_sequence; | ||
| } tsk_tree_position_t; | ||
|
|
||
| int tsk_tree_position_init( |
There was a problem hiding this comment.
Methods are declared further down the file, separate from struct definitions. Just move the function signatures to the bottom of the file.
|
@jeromekelleher, I've added a commit with all your suggested changes. The docs are still failing after moving all the methods to the bottom of |
duncanMR
force-pushed
the
tree_pos_revision
branch
from
133dfeb to
7307a85
Compare
|
I think the docs build is working @duncanMR - I just checked it out and the build is fine for me. Also the CI docs build is working. I think the CI failures are due to a stale package cache. Probably simplest to just start a new PR from a fresh branch. |
Yes, I figured out the docs problem: there is a hardcoded exception in the Sphinx config specifically for the tree class that needed an update. I'll make a new PR now |
|
I just saw a "merge" here @duncanMR - merge is not your friend! Rebase rebase rebase |
|
@jeromekelleher Sorry about that! I accidentally pulled changes instead of just fetching. Will be more careful in future |
No need to apologise, just trying to help |
Description
This PR aims to address the first part of #2795 by adding the
tree_positionclass to thetsk_tree_tstruct. I've done a first draft of this and it passes all the unit tests without any memory issues detected by Valgrind. I'm not confident that the memory management is correct yet, though!PR Checklist: