Skip to content

Drop headers#375

Merged
jakirkham merged 10 commits intozarr-developers:mainfrom
jakirkham:drop_headers
Oct 29, 2022
Merged

Drop headers#375
jakirkham merged 10 commits intozarr-developers:mainfrom
jakirkham:drop_headers

Conversation

@jakirkham
Copy link
Copy Markdown
Member

@jakirkham jakirkham commented Oct 28, 2022 •
edited
Loading

Convert code from header files to Cython (generated C code is equivalent). Then drop the headers. These were there primarily for Windows compatibility with old Python versions (namely 2.7), but shouldn't be needed any more.

xref: #368
xref: #369

TODO:

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

@jakirkham jakirkham force-pushed the drop_headers branch 12 times, most recently from a0fc296 to d3df03f Compare October 28, 2022 22:08
@jakirkham jakirkham requested a review from joshmoore October 28, 2022 22:24
@jakirkham
Copy link
Copy Markdown
Member Author

jakirkham commented Oct 28, 2022

@DimitriPapadopoulos, Idk how familiar you are with Cython/C, but would appreciate your review here (if you have time 🙂)

@jakirkham
Ignore Cython generated source files in numcodecs
fc75603
@jakirkham
Capitalize Fs in hex values
2deca35
@jakirkham
Wrap load_le32 over multiple lines
a8592e5 
Try to match `store_le32` and improve readability.
@jakirkham
Replace x & y with i & j
9d9e0e4 
Given these variables are integers, the latter is the more typical
naming convention.
@jakirkham
Use stdint types in store_le32/load_le32
95963d9
@jakirkham
Replace UINT32_SIZE with sizeof(uint32_t)
94ec0b2
@jakirkham
Implement store_le32 & load_le32 in Cython
d3250ae
@jakirkham
Drop stdint_compat.h
2fb1d41 
Everything that was relevant here has been moved to Cython. Also Cython
has its own `stdint` wrappers to handle differences encountered. So
those are not needed.

Plus this was only needed for old Visual Studio versions, which are not
used any more. In particular this was used with Python 2.7, which was
dropped from the codebase.
@jakirkham
Drop numcodecs headers from MANIFEST.in
fbc234a 
Since the only header file included has been dropped, there is no need
to include header files. So drop this from the `MANIFEST.in` as well.
@jakirkham
Ignore Cython generated header files in numcodecs
7e69f0c
@DimitriPapadopoulos
Copy link
Copy Markdown
Contributor

DimitriPapadopoulos commented Oct 29, 2022

I'll have a look, but I actually don't know much about Cython. I am more experienced in Python bindings for C++ libraries using SIP.

DimitriPapadopoulos
DimitriPapadopoulos reviewed Oct 29, 2022
View reviewed changes
.gitignore

# C extensions
numcodecs/**/*.c
numcodecs/**/*.h
Copy link
Copy Markdown
Contributor

@DimitriPapadopoulos DimitriPapadopoulos Oct 29, 2022

Choose a reason for hiding this comment

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

Had forgotten that. Thanks!

Copy link
Copy Markdown
Member Author

@jakirkham jakirkham Oct 29, 2022

Choose a reason for hiding this comment

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

No worries. Did as well. Plus being able to filter the headers meant needing to drop the one that was here.

@DimitriPapadopoulos
Copy link
Copy Markdown
Contributor

DimitriPapadopoulos commented Oct 29, 2022 •
edited
Loading

I think I get it:

  • Use uint8_t/uint32_t instead of char/int, this clarifies the intent and avoids any accidental bad cast (95963d9).
  • Most of the contents of stdint_compat.h are moved to _utils.pxd (d3250ae).
  • The now (almost) empty stdint_compat.h is dropped (2fb1d41).

It looks goods. 👍

DimitriPapadopoulos
DimitriPapadopoulos reviewed Oct 29, 2022
View reviewed changes
numcodecs/_utils.pxd
from libc.stdint cimport uint8_t, uint32_t


cdef inline void store_le32(uint8_t c[4], uint32_t i) nogil:
Copy link
Copy Markdown
Contributor

@DimitriPapadopoulos DimitriPapadopoulos Oct 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Adding nogil is good.

Copy link
Copy Markdown
Member Author

@jakirkham jakirkham Oct 29, 2022

Choose a reason for hiding this comment

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

Yep this forces Cython to generate straight C code. So the resulting code is nearly identical to what was in the header.

@jakirkham
Copy link
Copy Markdown
Member Author

jakirkham commented Oct 29, 2022

Yeah the dropping Python 2.7 & C files in your other two PRs reminded me there was some leftover compatibility code in this header for Windows Python 2.7. So this cleans that out and consolidates things into Cython (with some readability/code quality improvements).

joshmoore
joshmoore reviewed Oct 29, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@joshmoore joshmoore left a comment

Choose a reason for hiding this comment

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

No strong opinions from my side, so generally 👍.

Probably my only question is whether or not there will be any impact on client code. E.g. if stdint_compat.h doesn't get re-generated, could anyone have been depending on it ❓

@tacaswell tacaswell mentioned this pull request Oct 29, 2022
Closed
@jakirkham
Copy link
Copy Markdown
Member Author

jakirkham commented Oct 29, 2022

Thanks Josh! 🙏

Nope the header isn't installed. It is only used as part of the build process.

@jakirkham jakirkham merged commit 61a90da into zarr-developers:main Oct 29, 2022
@jakirkham jakirkham deleted the drop_headers branch October 29, 2022 20:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@joshmoore joshmoore joshmoore left review comments

+1 more reviewer

@DimitriPapadopoulos DimitriPapadopoulos DimitriPapadopoulos approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@jakirkham @DimitriPapadopoulos @joshmoore