Skip to content

Conversation

@jakirkham
Copy link
Member

Previously HEADER_LENGTH was defined in vlen, but not as const. So it could concievably be changed. While this didn't happen in practice, it does mean the compiler needs to generate code to check and use this value throughout.

However HEADER_LENGTH is intended to be const. So define it as such. That way the compiler can generate more efficient code when using it. Further define it in terms of the type whose size it references (namely uint32_t). Also make sure that type is cimported for referencing.

Finally consistently use HEADER_LENGTH when it applies throughout vlen. Previously HEADER_LENGTH was sometimes used and in other cases the value 4 was used. So consistently use HEADER_LENGTH throughout for improved code clarity. Now that HEADER_LENGTH is marked const this is equally efficient.


TODO:

  • Unit tests and/or doctests in docstrings
  • Tests pass locally
  • Docstrings and API docs for any new/modified user-facing classes and functions
  • Changes documented in docs/release.rst
  • Docs build locally
  • GitHub Actions CI passes
  • Test coverage to 100% (Codecov passes)

Previously `HEADER_LENGTH` was defined in `vlen`, but not as `const`. So
it could concievably be changed. While this didn't happen in practice,
it does mean the compiler needs to generate code to check and use this
value throughout.

However `HEADER_LENGTH` is intended to be `const`. So define it as such.
That way the compiler can generate more efficient code when using it.
Further define it in terms of the type whose size it references (namely
`uint32_t`). Also make sure that type is `cimport`ed for referencing.

Finally consistently use `HEADER_LENGTH` when it applies throughout
`vlen`. Previously `HEADER_LENGTH` was sometimes used and in other
cases the value `4` was used. So consistently use `HEADER_LENGTH`
throughout for improved code clarity. Now that `HEADER_LENGTH` is marked
`const` this is equally efficient.
@jakirkham jakirkham force-pushed the def_const_header_vlen branch from ac3cbab to 0a99c43 Compare April 7, 2025 20:20
@codecov
Copy link

codecov bot commented Apr 7, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.96%. Comparing base (96086c3) to head (e46d36e).
Report is 13 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #723   +/-   ##
=======================================
  Coverage   99.96%   99.96%           
=======================================
  Files          63       63           
  Lines        2738     2738           
=======================================
  Hits         2737     2737           
  Misses          1        1           
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@jakirkham jakirkham enabled auto-merge (squash) April 7, 2025 21:48
@jakirkham jakirkham merged commit 169db1b into zarr-developers:main Apr 7, 2025
30 checks passed
@jakirkham jakirkham deleted the def_const_header_vlen branch April 7, 2025 21:49
@jakirkham
Copy link
Member Author

jakirkham commented Apr 7, 2025

Weird would have thought automerge would have waited for the rest of the checks

Will keep an eye on main to confirm this does indeed pass (though it had earlier before resolving conflicts with the news entries)

Edit: Think this is because a small number of our checks are actually required. It may be worth revisiting this

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