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 for MSVC C4267 warning on ARM64 (which becomes error C2220 with /WX) #3320

Merged
merged 2 commits into from
Dec 5, 2022

Conversation

cwoffenden
Copy link
Contributor

I've only seen this on the ARM64 toolchain under the new ARM64 version of VS 2022 (running on a shiny new Windows Dev Kit 2023), and didn't see it cross-compiling for ARM64 from x64:

warning C4267: '-=': conversion from 'size_t' to 'int', possible loss of data
warning C4267: 'initializing': conversion from 'size_t' to 'int', possible loss of data

The change is minimal: i can't be a size_t because it needs to be signed, but changeing chunkSize to int is fine (it's only ever going to be 32 or 64 (or possibly somewhere between 16 to 128 on the more exotic systems).

(I can't verify the warning on x64 to ARM64 until next week but it's such a minor unintrusive change with known values)

I don't believe the (x64) Mac failure is related to error since it would take the SSE path.
@cwoffenden
Copy link
Contributor Author

Just to add, with the latest VS 2022, x86 host cross compiling to Windows ARM64 has the same warning without this fix.

@Cyan4973
Copy link
Contributor

cc @embg :
do we have ways to force compilation of the row matchfinder with the SWAR code path ?
If yes, is this code path already tested in CI ?

@aqrit
Copy link
Contributor

aqrit commented Nov 22, 2022

Fuzzer test with no intrinsics #2678

Copy link

@shakeebshams shakeebshams left a comment

Choose a reason for hiding this comment

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

LGTM

@Cyan4973 Cyan4973 merged commit 80cf73f into facebook:dev Dec 5, 2022
@cwoffenden cwoffenden deleted the msvc-arm64-size_t branch December 5, 2022 06:31
@Cyan4973 Cyan4973 mentioned this pull request Feb 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants