Skip to content

json+struct: padding bytes and no C code#3440

Open
petrelharp wants to merge 1 commit intotskit-dev:mainfrom
petrelharp:json-struct-no-c-code
Open

json+struct: padding bytes and no C code#3440
petrelharp wants to merge 1 commit intotskit-dev:mainfrom
petrelharp:json-struct-no-c-code

Conversation

@petrelharp
Copy link
Copy Markdown
Contributor

@petrelharp petrelharp commented Mar 20, 2026

This differs from #3437 in that:

  • it removes all C code from tskit
  • it adds some documentation for the json+struct codec

My intention is that we deal with the python and C code separately, so this supercedes #3437. My proposal for the C side of things is in #3439.

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 20, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.90%. Comparing base (2f26dc6) to head (dee7859).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3440      +/-   ##
==========================================
- Coverage   91.92%   91.90%   -0.02%     
==========================================
  Files          37       37              
  Lines       32153    32095      -58     
  Branches     5143     5135       -8     
==========================================
- Hits        29556    29498      -58     
  Misses       2264     2264              
  Partials      333      333              
Flag Coverage Δ
C 82.64% <ø> (-0.07%) ⬇️
c-python 77.54% <ø> (+0.19%) ⬆️
python-tests 96.40% <100.00%> (+<0.01%) ⬆️
python-tests-no-jit 33.21% <0.00%> (-0.01%) ⬇️

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

Components Coverage Δ
Python API 98.69% <100.00%> (+<0.01%) ⬆️
Python C interface 91.23% <ø> (ø)
C library 88.82% <ø> (-0.04%) ⬇️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@petrelharp petrelharp force-pushed the json-struct-no-c-code branch from 5363644 to 3d0f794 Compare March 21, 2026 15:23
@petrelharp petrelharp requested a review from benjeffery March 22, 2026 16:44
@petrelharp petrelharp marked this pull request as ready for review April 1, 2026 13:38
Copy link
Copy Markdown

@bhaller bhaller left a comment

Choose a reason for hiding this comment

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

stopping for now since I suspect I'm somehow looking at the wrong diffs

Copy link
Copy Markdown

@bhaller bhaller left a comment

Choose a reason for hiding this comment

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

next review

@petrelharp petrelharp requested a review from bhaller April 3, 2026 05:05
@petrelharp
Copy link
Copy Markdown
Contributor Author

elsewhere @bhaller points out that

OK; but since you're adding 4-byte ints, you’re not really testing the different padding byte counts very effectively.  You’re only testing the 0 and 4 alignment cases well, which seems vulnerable to bugs since it’s just “none” and “half”.

I don't think having the length of the binary bit be divisible by 4 is a problem, since the number of padding bytes is determined by the length of the JSON (which comes first). I'll add a few more string lengths to make sure we hit everything.

@petrelharp petrelharp force-pushed the json-struct-no-c-code branch from 9323072 to bbe839f Compare April 3, 2026 23:41
Copy link
Copy Markdown

@bhaller bhaller left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the revisions. Note the two comments that maybe you missed, but they are trivial so there's no need to hold things up for them. :->

@petrelharp petrelharp force-pushed the json-struct-no-c-code branch from bbe839f to dee7859 Compare April 4, 2026 00:26
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.

2 participants