Skip to content

Harden byte order detection#154

Merged
joto merged 2 commits into
masterfrom
kk/endian-detection
Jul 2, 2026
Merged

Harden byte order detection#154
joto merged 2 commits into
masterfrom
kk/endian-detection

Conversation

@kkaefer

@kkaefer kkaefer commented Jul 1, 2026

Copy link
Copy Markdown
Member

Use __BYTE_ORDER__ for GCC/Clang, and include Linux/BSD-specific headers for endian detection, and a constexpr static_assert instead of silently defaulting to little-endian.

Fixes #135.

@kkaefer kkaefer requested a review from a team as a code owner July 1, 2026 09:09
@kkaefer kkaefer requested a review from joto July 1, 2026 09:09
@kkaefer

kkaefer commented Jul 1, 2026

Copy link
Copy Markdown
Member Author

Clang and GCC define __BYTE_ORDER__ by default: https://gcc.gnu.org/onlinedocs/gcc-4.9.0/cpp/Common-Predefined-Macros.html

Use __BYTE_ORDER__ for GCC/Clang, and include Linux/BSD-specific headers for endian detection.

Fixes #135.
@kkaefer kkaefer force-pushed the kk/endian-detection branch from 0e2208d to 47f522d Compare July 1, 2026 11:59
@joto

joto commented Jul 1, 2026

Copy link
Copy Markdown
Collaborator

If the is_little_endian() function can perfectly detect the endianness at compile time, why don't we get rid of all the macro shenenigans alltogether? The only thing this is used for is to call (or not) the byteswap_inplace() function. If we add an if (!is_little_endian()) condition inside that function, we should be okay. We can use if consexpr if we switch to c++17 (which I don't want to do because libosmium is still c++14, but it's not out of the question to do that step). But even with a simple if the compiler should be able to remove all that code in the little-endian case because it detects that is superfluous.

@kkaefer

kkaefer commented Jul 1, 2026

Copy link
Copy Markdown
Member Author

If the is_little_endian() function can perfectly detect the endianness at compile time, why don't we get rid of all the macro shenenigans alltogether?

We can't. It didn't work, and I removed it again after running actual big endian machines where this failed.

@joto

joto commented Jul 1, 2026

Copy link
Copy Markdown
Collaborator

We can't. It didn't work, ...

Okay, then I am fine with it once the CI problems are fixed.

Introduce new CI job for building and testing on big-endian platforms (s390x).
@kkaefer kkaefer force-pushed the kk/endian-detection branch from 47f522d to 7c5d19b Compare July 2, 2026 09:11
@kkaefer

kkaefer commented Jul 2, 2026

Copy link
Copy Markdown
Member Author

Fixed the build.

@joto joto merged commit 7711edd into master Jul 2, 2026
67 checks passed
@kkaefer kkaefer deleted the kk/endian-detection branch July 2, 2026 11:26
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.

config.hpp's endian test silently falls back to little-endian

2 participants