Skip to content
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

Have default hash function returns a digest #13

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kamikat
Copy link

@kamikat kamikat commented Apr 10, 2016

A more consistent signature of a default hash function.

@mavam
Copy link
Owner

mavam commented Apr 10, 2016

The change to digest makes sense to me.

But you're introducing another subtle change silently: changing the digest type from size_t to uint64_t. So far, I'm going with the design of std::hash, which also returns size_t as a digest type, which is the most efficient unsigned type for a given architecture.

Frankly, the whole hashing design needs a major overhaul according to Howard's proposal.

@kamikat
Copy link
Author

kamikat commented Apr 11, 2016

Oh... yes, the commit includes a change of digest's typedef to uint64_t to help clarify a storage property of digest. I'd better commit that separately.

size_t is semantically interpreted as a return type of sizeof, which should not be an appropriate type to describe a hash result. A digest type should be a better solution to that problem.

It comes to the definition of a digest. A type signature of uint64_t describes the storage property, which can help developer understanding digest type.

@kamikat kamikat closed this Apr 11, 2016
@kamikat kamikat mentioned this pull request Apr 11, 2016
@mavam
Copy link
Owner

mavam commented Apr 11, 2016

size_t is semantically interpreted as a return type of sizeof, which should not be an appropriate type to describe a hash result. A digest type should be a better solution to that problem.

I'm not saying that size_t is the best choice, it's just what's used by std::hash to date, and that was the rationale to go with it intially. If you look at the design of the standard library, you will see that size_t is used for all sorts of "size" related computations, e.g., std::vector<T>::size.

I fully agree that size_t should not be the digest type, it just lends itself as natural fit in the very awkward design of the C++ hashing.

I will merge the PR if you remove the single uint64_t typedef. And thanks for contributing! :)

@kamikat
Copy link
Author

kamikat commented Apr 11, 2016

Thank you for the explanation. It seems that should be a design issue about C++ hashing.

@kamikat kamikat changed the title Change default hash function returns to a 'digest' Have default hash function returns a digest Apr 11, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants