Uses built-in byte swap if available (Apple) and if bswap_XX is undefined.
Defers to pre-defined version if found (e.g. protobuf). For protobuf case, the definitions are identical and thus include order should not affect results.
Fixes compilation errors on recent Mac OS X with QT client enabled.
Yes, this looks better, thanks
0 | @@ -0,0 +1,26 @@ 1 | +// Copyright (c) 2016 The Bitcoin Core developers
I'd prefer moving this one to the qt unit tests, instead of adding a qt-specific unit test to the core unit tests.
I moved this into a new "CompatTests" test suite in the QT tests.
Hmm, is it really necessary to have the system-specific bswaps anymore? Since we require a modern compiler, these are likely to be detected/optimized automatically anyway.
Test file: https://gist.github.com/theuni/deb0b97c31890bd0a06a65dd3cb51822
compiled with gcc/clang: c++ -std=c++11 -O2 -c -o bswapcomp.o bswap.cpp
Results: https://gist.github.com/theuni/508b69da908be1e52bbdd0b7a135c318
On x86-64, I see no difference for osx+clang, linux+clang, linux+gcc.
doesn't look like it's worth bothering to me, or is there some platform that would be more likely to be a discrepancy?
@theuni So you think it's better to basically #undef and #define them always for consistency?
Edit: I meant to say that since we want the same thing to happen no matter which include order is taken, we might as well just redefine them each time on our own side.
@kallewoof I was suggesting just skipping the platform includes (byteswap.h/OSByteOrder.h), skipping the use of any system implementations, and renaming bswap_foo -> int_bswap_foo to avoid collisions.
Need others to chime in first though.
FWIW I like the idea personally.
Defers to pre-defined version if found (e.g. protobuf). For protobuf case, the definitions are identical and thus include order should not affect results.
utACK https://github.com/bitcoin/bitcoin/pull/9366/commits/815f4148b2eff6c64c764e910e79677d5a67adc7 for the sake of fixing the build.
Dropping the system includes (or alternatively autoconfing this) can come as a next step if it's agreed upon.
Hmm, is it really necessary to have the system-specific bswaps anymore? Since we require a modern compiler, these are likely to be detected/optimized automatically anyway.
If there are bswap primitives, I think we should use them. This is used internally in the serialization as well as crypto operations, so performance here (if there is a compiler where it makes sense) does matter. If you want to prove for every compiler, architecture combo in existence that this doesn't matter, fine. But if not, I'd prefer to keep using system primitives if available.
utACK https://github.com/bitcoin/bitcoin/pull/9366/commits/815f4148b2eff6c64c764e910e79677d5a67adc7, let's just fix this to fix the build, I think we've been through enough shed-painting here.
FYI, I backported this to 0.13. I noticed it when trying to compile the Elements Alpha refresh.