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

Jump to bottom

bytes/hash: rename to hash/maphash #34778

Closed
rsc opened this issue Oct 8, 2019 · 10 comments
Closed

bytes/hash: rename to hash/maphash #34778

rsc opened this issue Oct 8, 2019 · 10 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Milestone
Go1.14

Comments

@rsc
Copy link
Contributor

rsc commented Oct 8, 2019

The implementation landed for #28322, which is great (thanks!).
The choice of bytes/hash as the import path is probably not right.

Based on the discussion on #28322, it's not completely clear it will
stay limited to bytes for all time. Even if it did, it is unclear why it is
bytes/hash and not hash/bytes. Neither is great, since the package
name alone shadows either bytes or hash. As it is, the current
bytes/hash.Hash implements (non-bytes/)hash.Hash, which is
pretty confusing. Goimports won't know what to do for hash.Hash
anymore, and so on.

It seems like it should be either in package hash, with a clear name
(hash.Seeded? hash.Random?),
or else in a hash/something package with a clear name.

/cc @randall77 @bradfitz @alandonovan
@rsc rsc added this to the Go1.14 milestone Oct 8, 2019
@rsc rsc added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Oct 8, 2019
@randall77
Copy link
Contributor

We sort of settled on a byte stream API, so bytes/hash seemed reasonable. Maybe hash/stream?

The hashes in hash are all explicitly spec'd hashes, which this really isn't. Thus I don't think it should just be another type in hash.

@rsc
Copy link
Contributor Author

rsc commented Oct 9, 2019

The fact that it's not spec'ed and can't escape the current program seems like the most important property, not the "stream" part. All hashes are streams. Maybe hash.Local or hash.Private or hash.Unspecified or hash.InMemory or hash.Undefined or ...
Just not hash.Fast.

@andybons andybons mentioned this issue Oct 9, 2019
Open
@bcmills
Copy link
Contributor

bcmills commented Oct 9, 2019

(Just bikeshedding, feel free to ignore.)

  • hash.Builtin?
  • hash.Runtime?

@alandonovan
Copy link
Contributor

Not Runtime -- we don't guarantee it's the same one used by Go maps.

If Fast is a bad name because it draws users to it like magpies to a shiny thing, then Unspecified is a good name.

@OneOfOne
Copy link
Contributor

OneOfOne commented Oct 9, 2019

hash.Memory? imho Unspecified sounds "scary" to a point, specially if you're coming from C/C++.

@rsc
Copy link
Contributor Author

rsc commented Oct 14, 2019

unspecified is such a long name to type.

hash/unspec
unspec.New
unspec.Hash
unspec.Seed

better ideas?

@rsc
Copy link
Contributor Author

rsc commented Oct 14, 2019

or hash/maphash?

// Package maphash implements a fast, randomly seeded, collision-resistant hash function
// for use in implementing hash table-based maps. The exact algorithm is unspecified and
// may vary from implementation to implementation. The random seeds make it impossible to
// depend on (or attack) the details of the algorithm anyway.

@josharian
Copy link
Contributor

josharian commented Oct 14, 2019 •
edited

Short: hash.Mem, hash.Go (or hash/memhash, hash/gohash)

@rsc
Copy link
Contributor Author

rsc commented Oct 30, 2019

It looks like there is a strong consensus for hash/maphash. Let's go with that.

@rsc rsc changed the title bytes/hash: unfortunate import path bytes/hash: rename to hash/maphash Oct 30, 2019
@rsc rsc added the NeedsFix The path to resolution is known, but the work has not been done. label Oct 30, 2019
@gopherbot gopherbot removed the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Oct 30, 2019
@gopherbot
Copy link

Change https://golang.org/cl/204997 mentions this issue: hash/maphash: move bytes/hash to hash/maphash

@golang golang locked and limited conversation to collaborators Nov 1, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Projects
None yet
Milestone
Go1.14
Development

No branches or pull requests
8 participants
@bradfitz @josharian @rsc @OneOfOne @bcmills @randall77 @alandonovan @gopherbot