Skip to content

Add Windows endian conversions - #87

Open
kungfubeaner wants to merge 1 commit into
fmang:masterfrom
kungfubeaner:pr1-endianness
Open

Add Windows endian conversions#87
kungfubeaner wants to merge 1 commit into
fmang:masterfrom
kungfubeaner:pr1-endianness

Conversation

@kungfubeaner

@kungfubeaner kungfubeaner commented Aug 22, 2026

Copy link
Copy Markdown

Adds the Windows-specific endian conversion support needed for the Windows port.

Provides htobe32() / be32toh() using compiler byte-swap intrinsics.
Provides htole32() / le32toh() for Windows.
Keeps the existing platform-specific implementations unchanged on Linux/macOS.

This is the first part of the Windows port and is intended to be followed by separate PRs for file handling and other Windows compatibility work.

@fmang fmang left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Thank you for the splitting! The code looks reasonable, but the documentation is a bit off the point (like an LLM’s rambling, to be frank).

I’ll look at the rest later this week.

Comment thread src/opustags.h
// Windows has no <endian.h>. It's always little-endian in practice, so the
// LE conversions are no-ops; for BE, use the compiler's byte-swap builtin
// rather than hand-rolled masking/shifting. Both MinGW-w64 (GCC/Clang) and
// MSVC provide one.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

The assumption that Windows is always little-endian is worth stating, but that we use the compiler’s function instead of performing the bit operations ourselves is clear by reading the code. Unless you explain why, that mention is useless. If you wish to say that MSVC provides _byteswap_ulong, you should put the comment below, in the #else block. Just saying it provides one without giving the name of the provided function is vague.

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