Skip to content
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

Fix build error on MSVC #177

Merged
merged 1 commit into from
Dec 31, 2024
Merged

Fix build error on MSVC #177

merged 1 commit into from
Dec 31, 2024

Conversation

cgohlke
Copy link
Contributor

@cgohlke cgohlke commented Dec 30, 2024

Fixes the following syntax errors when building with Microsoft Visual C compiler:

src/bitshuffle_core.c(52): error C2061: syntax error: identifier '__attribute__'
src/bitshuffle_core.c(52): error C2059: syntax error: ';'

Re #161.

Fixes the following syntax errors when building with Microsoft Visual C compiler:
```
src/bitshuffle_core.c(52): error C2061: syntax error: identifier '__attribute__'
src/bitshuffle_core.c(52): error C2059: syntax error: ';'
```
@kiyo-masui
Copy link
Owner

Passes the CI which is good.

I'm not very familiar with what the aliasing rules and optimizations actually do, but from a quick search it does seem like changes in behavior are possible (its not just speed). So just to confirm, do you understand what this change actually does, and that you are confident that it will not lead to incorrect behavior on MVC? Obviously this shouldn't change gcc compilations, as confirmed by the CI, but the CI does not cover MVC.

@cgohlke
Copy link
Contributor Author

cgohlke commented Dec 31, 2024

My understanding of #161 is that it only affects the NEON code path (not used on Windows) and the SSE code path previously used uint16_t anyway. I'm using this patch in the vendored version of bitshuffle in the imagecodecs package and have not noticed any test failures, even on win-arm64.

@kiyo-masui
Copy link
Owner

Fair enough!

@kiyo-masui kiyo-masui merged commit 526440a into kiyo-masui:master Dec 31, 2024
9 checks passed
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