Skip to content

64 bit tsk_size_t #1618

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.

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

Merged
merged 1 commit into from
Aug 31, 2021
Merged

64 bit tsk_size_t #1618

merged 1 commit into from
Aug 31, 2021

Conversation

jeromekelleher
Copy link
Member

@jeromekelleher jeromekelleher commented Aug 9, 2021

I started making lots of casts for the various library functions, but then realised that this could actually hide bugs in some cases. So, adding wrappers for the various functions seemed like the best approach. Most of these should probably be macros, but we can figure out the details of this a bit later if we agree on the basic approach.

@benjeffery, what do you think?

Closes #1530
Closes #1527
Closes #1554

@benjeffery
Copy link
Member

@jeromekelleher I think this way is better than lots of casts, and as you point out allows us to encapsulate other complexity like avoiding zero-allocs 👍

@jeromekelleher
Copy link
Member Author

OK, thanks @benjeffery. I think I'll try to cherry pick some of this stuff out so that it'll compile cleanly on 64 bit platforms, and they incorporate the other diffs into the 64 bit switch.

@jeromekelleher jeromekelleher force-pushed the size_t_32bit branch 2 times, most recently from d1ff176 to 2406ddc Compare August 13, 2021 05:10
@jeromekelleher
Copy link
Member Author

jeromekelleher commented Aug 13, 2021

I think this is basically working @benjeffery, and we're possibly at the point where we should compile a list of follow-up issues and merge. I think this is all the critical stuff. I've we're happy with this set of changes, I think I'll squash down to one commit so that we're cleanly compilable on 32 and 64 bit if we ever need to git bisest.

What do you think?

@codecov
Copy link

codecov bot commented Aug 13, 2021

Codecov Report

Merging #1618 (643331b) into main (0d2dad9) will increase coverage by 0.03%.
The diff coverage is 100.00%.

❗ Current head 643331b differs from pull request most recent head 822ba61. Consider uploading reports for the commit 822ba61 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1618      +/-   ##
==========================================
+ Coverage   93.68%   93.72%   +0.03%     
==========================================
  Files          27       27              
  Lines       23306    23318      +12     
  Branches     1084     1084              
==========================================
+ Hits        21834    21854      +20     
+ Misses       1438     1430       -8     
  Partials       34       34              
Flag Coverage Δ
c-tests 91.87% <100.00%> (+<0.01%) ⬆️
lwt-tests 93.49% <100.00%> (+0.08%) ⬆️
python-c-tests 95.47% <100.00%> (+0.07%) ⬆️
python-tests 98.79% <ø> (-0.01%) ⬇️

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

Impacted Files Coverage Δ
c/tskit/core.h 100.00% <ø> (ø)
c/tskit/core.c 97.21% <100.00%> (ø)
c/tskit/genotypes.c 93.91% <100.00%> (ø)
c/tskit/haplotype_matching.c 95.05% <100.00%> (-0.02%) ⬇️
c/tskit/tables.c 89.92% <100.00%> (+<0.01%) ⬆️
c/tskit/trees.c 94.86% <100.00%> (+<0.01%) ⬆️
python/_tskitmodule.c 92.34% <100.00%> (+0.16%) ⬆️
python/lwt_interface/tskit_lwt_interface.h 95.41% <100.00%> (ø)
... and 3 more

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 0d2dad9...822ba61. Read the comment docs.

@jeromekelleher jeromekelleher changed the title Changes needed on 32 bit for 64 bit tsk_size_t 64 bit tsk_size_t Aug 13, 2021
Copy link
Member

@benjeffery benjeffery left a comment

Choose a reason for hiding this comment

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

LGTM, couple of questions! I agree, lets merge and then fix up the last few things.

Major change at all levels required to change the size of tsk_size_t
to be 64 bit, and to make this compile cleanly on 64 and 32 bit
machines.

Basic C library changes for 64 bit tsk_size_t

Update offset column accessors

Fix uint32 in sample_set_sizes

Misc size_t casts in library code

size_t changes for tests

Change tsk_xalloc shims to tsk_size_t

Remove problematic consts in lwt code

The numpy C API was discarding consts, so they are more trouble than
they are worth.
@mergify mergify bot merged commit 8fe8d2d into tskit-dev:main Aug 31, 2021
@jeromekelleher jeromekelleher deleted the size_t_32bit branch August 31, 2021 12:47
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.

64bit offsets - LWT compatibility Support 32 or 64 bit sizes in offset columns Change offset columns to 64 bit.
2 participants