NVIDIA / cuCollections

Apache License 2.0
448 stars 79 forks source link

Support any hash constructor in hash_test.cu::check_hash_result #520

Open esoha-nvidia opened 1 week ago

esoha-nvidia commented 1 week ago
          I see why you made this: Because we have hash functions that don't take a seed.  However, we'll eventually have the `SigBitHash` that takes not a seed but rather a list or a range or whatever.

I think that it would be better to not write a million of these functions. The right way to do it is with a variadic template. Also, you could make that a different PR. The code would look like this:

template <typename Hash, typename... HashConstructorArgs>
static __host__ __device__ bool check_hash_result(typename Hash::argument_type const& key,
                                                  typename Hash::result_type expected,
                                                  HashConstructorArgs&&... hash_constructor_args) noexcept
{
  Hash h(std::forward<HashConstructorArgs>(hash_constructor_args)...);
  return (h(key) == expected);
}

The seed is now the last argument so you need to find all occurrences of check_hash_result in this file and make the seed the last argument.

After doing this change, the function will now be compatible with all hash construction.


If you want to read more about std::forward, these are good:

The short of it is that std::forward written this way with a "universal reference" makes it so that this code works well when:

When used like this, the compiler will correctly optimize if the value of the seed or whatever is passed in there needs to be copied or moved.

_Originally posted by @esoha-nvidia in https://github.com/NVIDIA/cuCollections/pull/514#discussion_r1653534402_

sleeepyjack commented 1 week ago

I like the idea. We'll incorporate it into the test.