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

Make Multihash a string instead of a []byte? #29

Open
kevina opened this issue Sep 23, 2016 · 6 comments
Open

Make Multihash a string instead of a []byte? #29

kevina opened this issue Sep 23, 2016 · 6 comments

Comments

@kevina
Copy link
Contributor

kevina commented Sep 23, 2016

Currently Multihash is defined as:

type Multihash []byte

A multihash may be binary, but it is small and (I hope) immutable. In my view a string is a better fit since it will ensure that it will not change. It will also take less memory to point too (two pointers, instead of three).

This relates to ipfs/go-cid#3.

@whyrusleeping
Copy link
Member

The reason i vote in favor of using a []byte is because once we move to using a string, anything that needs to copy or write out a multihash needs to make a copy of it first, and those can get expensive.

@kevina
Copy link
Contributor Author

kevina commented Nov 17, 2016

I suppose that is true. It is really annoying that go does not have a concept of "const" as that can avoid most of these unnecessary copies, for example having to make a copy to pass a string into a function that expects a []byte even though there the function will not modify the data.

@kevina
Copy link
Contributor Author

kevina commented Oct 19, 2017

And the argument can also be that more things end up needing it as a string that a []byte. @Stebalien what are your thoughts on this manor.

Changing the type of Multihash will be a simple but API breaking change. This perhaps should be done at the same time as ipfs/go-cid#3 (if we decide to do that) to minimize the pain.

@Stebalien
Copy link
Member

I see CIDs used as keys in maps all the time and using a string would help there a lot. Honestly, I'm not sure (we'd have to test it).

@kevina
Copy link
Contributor Author

kevina commented Oct 19, 2017

@Stebalien I think you are you are confusing CID with mutlihashes, they are now two different things.

@Stebalien
Copy link
Member

Ah. Sorry, too many issues open (I care about this because it's one route to make CID's usable as map keys).

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

No branches or pull requests

3 participants