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 #378, bswap on read on big-endian architectures #432

Merged
merged 2 commits into from
Jul 12, 2019

Conversation

cary-ilm
Copy link
Member

@cary-ilm cary-ilm commented Jul 9, 2019

Every system seems to have a different implementation of bswap, is there a simpler way than this?

I don't have big-endian machine to test this, so just assuming this works.

Fixes #378

sharkcz and others added 2 commits July 2, 2019 14:11
testFutureProofing and testMultiPartFileMixingBasic both use fread(&length,4,f) to get a 4 byte
integer value from input file. The value read is not converted from the little endian format to
the machine format causing problems (eg. test didn't finish after 24 hours).

fixes issue AcademySoftwareFoundation#81
Copy link
Contributor

@meshula meshula left a comment

Choose a reason for hiding this comment

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

lgtm

@axxel
Copy link
Contributor

axxel commented Jul 11, 2019

You could use intrinsics based on the compiler instead of including platform specific headers. Both (recent versions of) clang and gcc provide __builtin_bswap32 and MSVC has _byteswap_ulong. See also https://stackoverflow.com/a/105339/1461424.

@meshula
Copy link
Contributor

meshula commented Jul 11, 2019

The file Cary added, bswap32.h, selects the built in intrinsics you haved noted. It's good to double check these things though, thanks.

@axxel
Copy link
Contributor

axxel commented Jul 11, 2019

I did not want claim that the mentioned builtins were 'different' or 'better'. It was a response to the question he posted in his first comment, i.e. I believe a 5-line ifdef would be simpler than his solution. That said, it might not work for each and every compiler (version) that his code works with.

The future is bright, though: http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p1272r0.html ;) (reference implementation: https://github.com/slurps-mad-rips/byteswap/blob/master/include/ixm/byteswap.hpp)

@cary-ilm
Copy link
Member Author

I'm going to leave this as is for now, if a simpler method comes along that works on the targeted systems without so many #if's, we can simplify it then.

@cary-ilm cary-ilm merged commit 225ddb8 into AcademySoftwareFoundation:master Jul 12, 2019
@cary-ilm cary-ilm deleted the fix-378 branch July 12, 2019 22:15
@sharkcz sharkcz mentioned this pull request Jul 13, 2019
@cary-ilm cary-ilm added this to the v2.4.0 milestone Aug 10, 2019
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.

None yet

4 participants