Skip to content

Conversation

@jeromekelleher
Copy link
Member

Closes #1979

The idea of this was to open the ground for #1977, but I'm having second thoughts now because the x_table_set_columns doesn't have an options value (which I forgot).

I guess we could add an option to tsk_x_table_init(table, options) which is like TSK_EXTERNAL_MEMORY or something, which flicks a switch were we store pointers in set_columns and set_metadata_schema, set_reference_sequence_data etc. So then, we don't need these options values.

I guess the alternative is to add options to all the x_tables_set_columns.

Any thoughts @benjeffery?

@jeromekelleher
Copy link
Member Author

Ah, I'd forgotten about the update case where we want to pass ownership of a large string to the table. This is easy to do, and definitely requires the options flag, so it's a done-deal that we need this. I'll update later.

@benjeffery
Copy link
Member

Adding options to x_tables_set_columns seems exactly the kind of thing we should be doing before 1.0, so makes sense to me.

@jeromekelleher
Copy link
Member Author

I was thinking the same thing...

@jeromekelleher jeromekelleher force-pushed the add-options-to-string-setters branch from a14abb4 to 26babdb Compare December 3, 2021 16:38
@jeromekelleher jeromekelleher marked this pull request as ready for review December 3, 2021 16:38
@jeromekelleher
Copy link
Member Author

OK, I think this is ready to go - might was well get the messy stuff out of the way before starting on #2006.

@jeromekelleher
Copy link
Member Author

Oops, needs a CHANGELOG entry...

@jeromekelleher jeromekelleher force-pushed the add-options-to-string-setters branch from 26babdb to 959a028 Compare December 3, 2021 16:44
@codecov
Copy link

codecov bot commented Dec 3, 2021

Codecov Report

Merging #1990 (959a028) into main (97e9dad) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1990   +/-   ##
=======================================
  Coverage   93.35%   93.35%           
=======================================
  Files          27       27           
  Lines       25521    25522    +1     
  Branches     1111     1111           
=======================================
+ Hits        23826    23827    +1     
  Misses       1660     1660           
  Partials       35       35           
Flag Coverage Δ
c-tests 92.37% <100.00%> (+<0.01%) ⬆️
lwt-tests 89.05% <100.00%> (ø)
python-c-tests 72.09% <100.00%> (ø)
python-tests 98.79% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
c/tskit/tables.c 90.26% <100.00%> (+<0.01%) ⬆️
python/_tskitmodule.c 90.94% <100.00%> (ø)
python/lwt_interface/tskit_lwt_interface.h 90.81% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 97e9dad...959a028. Read the comment docs.

@jeromekelleher jeromekelleher force-pushed the add-options-to-string-setters branch from d2b08a2 to 959a028 Compare December 3, 2021 17:27
@jeromekelleher jeromekelleher marked this pull request as draft December 3, 2021 19:34
@jeromekelleher
Copy link
Member Author

Ugh, maybe we don't want to do this. As noted in #2006 (comment) we'll have to drop all the const qualifiers in the signatures if we do it this way, which is particularly annoying when dealing with literal strings.

Maybe we should have a separate set of methods instead?

@jeromekelleher
Copy link
Member Author

Closed in favour of "takeset" methods, #2016

@jeromekelleher jeromekelleher deleted the add-options-to-string-setters branch December 5, 2021 16:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add options flag to all string setting methods

2 participants