Reference sequence - C API #1911
Conversation
|
WIP, but probably useful to check I am heading in the right direction. |
Codecov Report
@@ Coverage Diff @@
## main #1911 +/- ##
==========================================
- Coverage 93.36% 93.32% -0.04%
==========================================
Files 27 27
Lines 25058 25186 +128
Branches 1107 1107
==========================================
+ Hits 23395 23505 +110
- Misses 1629 1647 +18
Partials 34 34
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
benjeffery
force-pushed
the
ref_seq
branch
2 times, most recently
from
d7b522e to
ae8cb62
Compare
|
I think this is worth you looking at now @jeromekelleher |
jeromekelleher
left a comment
jeromekelleher
left a comment
There was a problem hiding this comment.
LGTM - some suggestions for moving things around so that most of the code is in the tsk_reference_sequence_t class, except for the table dump/load methods. These need to be in the table_collection class because we need only create a reference_sequence object if there is data to be loaded.
benjeffery
force-pushed
the
ref_seq
branch
2 times, most recently
from
1686d6b to
68c7963
Compare
|
Ok, after a very annoying double free I think this is ready to be looked at again. |
jeromekelleher
left a comment
jeromekelleher
left a comment
There was a problem hiding this comment.
Keeping track of whether the reference sequence is NULL or not is tricky and tedious, so I wonder if it would be simpler if we kept a state variable instead and embedded the reference_sequence_t struct in the table collection.
But let's merge this first - there's a few minor changes to make which hopefully make things follow the current idioms a bit better.
c/tskit/tables.c
Outdated
| tsk_safe_free(self->url); | ||
| tsk_safe_free(self->metadata); | ||
| tsk_safe_free(self->metadata_schema); | ||
| tsk_safe_free(self); |
There was a problem hiding this comment.
I see why you've done this but I think it'll just confuse us in the long term making this object a special case. It also rules out using a common idiom
tsk_reference_sequence_t tmp;
ret = tsk_reference_sequence_init(&tmp);
tsk_reference_sequence_free(&tmp);
| tsk_safe_free(self); | ||
| return 0; | ||
| } | ||
|
|
There was a problem hiding this comment.
| int | |
| tskreference_sequence_init(tsk_reference_sequence_t *self) | |
| { | |
| tskmemset(self, sizeof(*self)); | |
| return 0 | |
| } |
}
| tsk_reference_sequence_equals(const tsk_reference_sequence_t *self, | ||
| const tsk_reference_sequence_t *other, tsk_flags_t options) | ||
| { | ||
| return ( |
There was a problem hiding this comment.
This is a pretty big nested expression - maybe break it up to make it easier to read?
if (self == NULL && other == NULL) {
return true;
}
/* If one or the other is NULL they are not equal */
if (self == NULL != other == NULL) {
return false;
}
return (self->data_length == other->data_length ...)
d347bda to
1857803
Compare
That's how I originally had it! I've addressed the comments, working on the python side now. |
How about get this merged in first? There's some errors in circleic |
Fixed! |
2 participants
WIP - Looking into this I think we just do it.