RedSpah / xxhash_cpp

Port of the xxhash library to C++17.
BSD 2-Clause "Simplified" License
163 stars 31 forks source link

128-bit checksum compilation error with 0.8.1 #30

Closed bitbugprime closed 1 year ago

bitbugprime commented 1 year ago

Compiling this function, which worked with version 0.7.3. to_string() not given here, but what it does is reasonably obvious. Note the error is not in this function, but in the header:

std::string
xxhash3(std::string_view data)
{
#define checksum_bits 128
    static_assert(checksum_bits == 64 || checksum_bits == 128);

    xxh::hash3_state_t<checksum_bits> state;
    state.update(data.data(), data.size());

    // convert checksum to canonical byte order                                                                                     
    xxh::canonical_t<checksum_bits> const canonical{state.digest()};
    auto const hash{canonical.get_hash()};

#if checksum_bits == 128
    return to_string(hash.low64, hash.high64);
#else
    return to_string(hash);
#endif
}

g++ 8.3 in C++17 mode:

xxhash/0.8.1/include/xxhash.hpp: In function ‘xxh::uint128_t xxh::intrin::bit_ops::mult64to128(uint64_t, uint64_t)’:
xxhash/0.8.1/include/xxhash.hpp:290:10: error: request for member ‘low64’ in ‘r128’, which is of non-class type ‘__int128 unsigned’
     r128.low64 = (uint64_t)(product);
          ^~~~~
xxhash/0.8.1/include/xxhash.hpp:291:10: error: request for member ‘high64’ in ‘r128’, which is of non-class type ‘__int128 unsigned’
     r128.high64 = (uint64_t)(product >> 64);
          ^~~~~~
xxhash/0.8.1/include/xxhash.hpp:292:12: error: could not convert ‘r128’ from ‘__int128 unsigned’ to ‘xxh::uint128_t’ {aka ‘xxh::typedefs::uint128_t’}
     return r128;
            ^~~~
xxhash/0.8.1/include/xxhash.hpp: At global scope:
xxhash/0.8.1/include/xxhash.hpp:915:3: error: explicit template specialization cannot have a storage class
   static inline uint_t<32> avalanche<32>(uint_t<32> hash) {
   ^~~~~~
xxhash/0.8.1/include/xxhash.hpp:925:3: error: explicit template specialization cannot have a storage class
   static inline uint_t<64> avalanche<64>(uint_t<64> hash) {
   ^~~~~~
RedSpah commented 1 year ago

Just pushed an attempted fix for this into dev, lemme know if it works.

bitbugprime commented 1 year ago

This does indeed fix my compilation problem, thanks. Is the 0.8.1 upgrade expected to give different checksums than 0.7.3? I noticed that I get the same checksum in some cases and a different one in others.

RedSpah commented 1 year ago

It depends, I don't think it should be happening but it could also be a change in the C version that was just reflected in this update- it would be ideal if you would compare the checksum results the C version 0.7.3 and 0.8.1 give you to see if those differ too and whether they match the C++ version.

bitbugprime commented 1 year ago

My "different checksums" are the same as the 0.8.1 C xxhsum program (from Cyan4973/xxHash) gives when it uses the new (to 0.8.0) XXH3 algorithm. It looks like you implemented XXH3 for this release and it does indeed give different results in return for its claimed performance improvements.

RedSpah commented 1 year ago

In which case it's working as intended, right? Unless I'm missing something

bitbugprime commented 1 year ago

It's working fine, it's just good to confirm your design choice.