don't use pointers for Multihash.String

multiformats / go-multihash

Multihash implementation in Go

MIT License

239 stars 57 forks source link

don't use pointers for Multihash.String #148

Closed mvdan closed 3 years ago

mvdan commented 3 years ago

(see commit message)

mvdan commented 3 years ago

FYI @masih; ran into this as part of the indexer provider refactor.

Added a couple of other reviewers who can hopefully approve, since Steven seems to be on holiday this week.

mvdan commented 3 years ago

Also: I'm pretty sure this is a backwards-compatible API change. From the Go spec:

The method set of the corresponding pointer type T is the set of all methods declared with receiver T or T (that is, it also contains the method set of T).

That is, the method is still present in *Multihash; the change is that it's now also present for Multihash.

mvdan commented 3 years ago

No profilling/benchmarking; just the API UX. See the commit message.

I'm betting that avoiding the pointer means slightly better perf, but I also imagine it doesn't matter in practice. Especially when compared to the easier-to-use API.

mvdan commented 3 years ago

Thanks all for the speedy reviews :)