Skip to content

mem: handle Float and Bool cases in byteSwapAllFields - #20523

Merged
andrewrk merged 1 commit into
ziglang:masterfrom
kcbanner:byte_swap_scalar
Jul 19, 2024
Merged

mem: handle Float and Bool cases in byteSwapAllFields#20523
andrewrk merged 1 commit into
ziglang:masterfrom
kcbanner:byte_swap_scalar

Conversation

@kcbanner

@kcbanner kcbanner commented Jul 7, 2024

Copy link
Copy Markdown
Contributor

I was working on some serialization code, and noticed that the .Float and .Bool cases weren't handled in byteSwapAllFields. I moved the scalar cases into byteSwap and call them from byteSwapAllFields.

@kcbanner
kcbanner force-pushed the byte_swap_scalar branch from ae47ca6 to 5c5a93b Compare July 7, 2024 06:36
@nektro

nektro commented Jul 7, 2024

Copy link
Copy Markdown
Contributor

i dont agree that std.mem.byteSwap() should exist. the fix should remain localized to byteSwapAllFields. people should always prefer @byteSwap in the general case and they wont necessarily if this function exists

@kcbanner

kcbanner commented Jul 7, 2024

Copy link
Copy Markdown
Contributor Author

That's fair, I figured since there was common logic I would move it to that method. Also since @byteSwap doesn't work on floats, it means the user doesn't have to write that logic themselves.

I've moved the fix back into byteSwapAllFields.

@kcbanner
kcbanner force-pushed the byte_swap_scalar branch from 5c5a93b to ef23cd7 Compare July 7, 2024 19:25
@kcbanner kcbanner changed the title mem: add byteSwap, and handle the float and bool cases mem: handle Float and Bool cases in byteSwapAllFields Jul 7, 2024
@nektro

nektro commented Jul 7, 2024

Copy link
Copy Markdown
Contributor

That's fair, I figured since there was common logic I would move it to that method.

totally

Also since @byteSwap doesn't work on floats, it means the user doesn't have to write that logic themselves.

this might be worth a separate issue. there could be a reason why floats and enums are not supported or it could be a simple oversight and no one's mentioned it before. would love to learn more about the use case

@andrewrk
andrewrk merged commit 647a0c9 into ziglang:master Jul 19, 2024
@andrewrk andrewrk added standard library This issue involves writing Zig code for the standard library. release notes This PR should be mentioned in the release notes. labels Jul 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release notes This PR should be mentioned in the release notes. standard library This issue involves writing Zig code for the standard library.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants