No need to have a (naive) copy of the ReadLE64 logic inside uint256::GetUint64, when we have an optimized function for exactly that.
Use ReadLE64 in uint256::GetUint64 instead of duplicating logic #26105
pull sipa wants to merge 1 commits into bitcoin:master from sipa:202209_uint256_readle64 changing 1 files +2 −9-
sipa commented at 8:50 PM on September 15, 2022: member
-
Use ReadLE64 in uint256::GetUint64() instead of duplicating logic 04fee75bac
-
davidgumberg commented at 5:20 AM on September 16, 2022: contributor
ACK 04fee75bacb9ec3bceff1246ba6c8ed8a8759548
The existing code assumes host endianness.
-
jonatack commented at 9:14 AM on September 16, 2022: contributor
ACK 04fee75bacb9ec3bceff1246ba6c8ed8a8759548 review, this use of ReadLE64() is similar to the existing invocation by Num3072::Num3072(), sanity checked that before and after this change GetUint64() returns the same result (debug build, clang 13)
--- a/src/uint256.h +++ b/src/uint256.h @@ -7,6 +7,7 @@ #include <crypto/common.h> +#include <logging.h> #include <span.h> @@ -85,7 +86,20 @@ public: uint64_t GetUint64(int pos) const { - return ReadLE64(m_data + pos * 8); + const uint8_t* ptr = m_data + pos * 8; + uint64_t x = ((uint64_t)ptr[0]) | \ + ((uint64_t)ptr[1]) << 8 | \ + ((uint64_t)ptr[2]) << 16 | \ + ((uint64_t)ptr[3]) << 24 | \ + ((uint64_t)ptr[4]) << 32 | \ + ((uint64_t)ptr[5]) << 40 | \ + ((uint64_t)ptr[6]) << 48 | \ + ((uint64_t)ptr[7]) << 56; + LogPrintf("GetUint64 old: %s\n", x); + + const auto y = ReadLE64(m_data + pos * 8); + LogPrintf("GetUint64 new: %s\n", y); + + assert(x == y); + return y; }2022-09-16T09:13:53Z GetUint64 old: 2830827502690553650 2022-09-16T09:13:53Z GetUint64 new: 2830827502690553650 2022-09-16T09:13:53Z GetUint64 old: 13427380872610120290 2022-09-16T09:13:53Z GetUint64 new: 13427380872610120290 2022-09-16T09:13:53Z GetUint64 old: 12183277635375193944 2022-09-16T09:13:53Z GetUint64 new: 12183277635375193944 2022-09-16T09:13:53Z GetUint64 old: 16580367070613284548 2022-09-16T09:13:53Z GetUint64 new: 16580367070613284548 2022-09-16T09:13:53Z GetUint64 old: 16949092074016075712 2022-09-16T09:13:53Z GetUint64 new: 16949092074016075712 2022-09-16T09:13:53Z GetUint64 old: 14891467430351852074 2022-09-16T09:13:53Z GetUint64 new: 14891467430351852074 2022-09-16T09:13:53Z GetUint64 old: 2319828850847527094 2022-09-16T09:13:53Z GetUint64 new: 2319828850847527094 2022-09-16T09:13:53Z GetUint64 old: 921024485749193602 2022-09-16T09:13:53Z GetUint64 new: 921024485749193602 - fanquake merged this on Sep 16, 2022
- fanquake closed this on Sep 16, 2022
-
sipa commented at 12:16 PM on September 16, 2022: member
The existing code assumes host endianness.
No, it does not. Both the old and new code interpret the bytes as little-endian.
-
davidgumberg commented at 12:25 PM on September 16, 2022: contributor
The existing code assumes host endianness.
No, it does not. Both the old and new code interpret the bytes as little-endian.
Right, I meant that the old code interprets the bytes as little endian and stores them as little endian, even if the host machine is big-endian.
ReadLE64invokesle64tohwhich does not make an assumption about "host" endianness. - sidhujag referenced this in commit 2cdf7113f2 on Sep 20, 2022
- bitcoin locked this on Sep 16, 2023