librhash/byte_order.h: Consult also compiler's predefined macros. #25
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
byte_order.h includes <endian.h> only if glibc is used (i.e. __GLIBC__ is defined), but there are other libc implementation that also provide endian.h, like musl libc for instance. Generally it's much better and cleaner to check feature test macros than check for particular implementation (and that is also why musl libc doesn't define __MUSL__). Unfortunately I am not aware of feature test macro guaranteeing endian.h availability. The simple solution is to add checking for compiler's pre-defined macros (__BYTE_ORDER__, __ORDER_LITTLE_ENDIAN__, __ORDER_LITTLE_ENDIAN__) after checking macros coming from system headers, i.e. <endian.h> in this case (__BYTE_ORDER, __BIG_ENDIAN, __LITTLE_ENDIAN). That way we can better support new architectures w/o hardcoding their endianness, which is especially useful if they support both.
|
merged into main tree as 7e4a509 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Recently I created patch for Alpine Linux's aports to include rhash in its repository, because I find this tool very useful. Unfortunately building on aarch64 (64-bit ARM architecture) failed, but I don't have access to such hardware, so I couldn't check it earlier.
Investigating build logs made me realize that byte_order.h has too much hardcoded stuff, yet it fails work on "unknown" platforms with non-glibc libc implementations. I think that ultimately this header should be rewritten, but w/o having access to all the platforms to retest it there, I didn't go that way. Instead I simply added another way to check endianness that depends on compiler's provided macros (at least gcc and clang provide them), that are usually used by
<endian.h>anyway.After successfully building and running
tests/test_rhash.shon aarch64 (kudos to @clandmeter who has access to such hardware and was kind enough to do builds and run test), the patch has been added to Alpine Linux repository in testing/rhash and therefore rhash is available in edge's testing repository for now.Now I would like to upstream the improvement.
Commit message
byte_order.h includes
<endian.h>only if glibc is used (i.e.__GLIBC__is defined), but there are other libc implementation that also provide endian.h, like musl libc for instance. Generally it's much better andcleaner to check feature test macros than check for particular implementation (and that is also why musl libc doesn't define
__MUSL__). Unfortunately I am not aware of feature test macro guaranteeing endian.h availability.The simple solution is to add checking for compiler's pre-defined macros (
__BYTE_ORDER__,__ORDER_LITTLE_ENDIAN__,__ORDER_LITTLE_ENDIAN__) after checking macros coming from system headers, i.e.<endian.h>in this case (__BYTE_ORDER,__BIG_ENDIAN,__LITTLE_ENDIAN).That way we can better support new architectures w/o hardcoding their endianness, which is especially useful if they support both.