-
Notifications
You must be signed in to change notification settings - Fork 85
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
xxhash cleanups #313
xxhash cleanups #313
Conversation
@@ -86,20 +86,20 @@ struct XXHash_32 { | |||
constexpr result_type __host__ __device__ operator()(Key const& key) const noexcept | |||
{ | |||
// TODO do we need to add checks/hints for alignment? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can someone comment on this? I'm not sure if the excessive reinterpret_cast
ing leads to pitfalls.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @jrhemstad
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on the offline discussion, downcasting to the char pointer shouldn't cause any problems and blocks
(or the uint32_t
pointer) won't be used if the key size is smaller than 4 bytes so the current implementation should be safe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
8c4f167 sprinkles in some [[maybe_unused]]
attributes for good measure.
@@ -86,20 +86,20 @@ struct XXHash_32 { | |||
constexpr result_type __host__ __device__ operator()(Key const& key) const noexcept | |||
{ | |||
// TODO do we need to add checks/hints for alignment? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @jrhemstad
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This PR is a follow-up on #310 and addresses review comments from @bdice.