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

proposal: encoding: BinaryMarshaler should be an append API #24630

Open
FiloSottile opened this Issue Apr 1, 2018 · 5 comments

Comments

Projects
None yet
8 participants
@FiloSottile
Copy link
Member

FiloSottile commented Apr 1, 2018

The BinaryMarshaler API returns a freshly allocated byte slice. This makes it unadvisable to implement for high performance APIs, or even APIs that could happen to be used in high performance scenarios.

If it had an append-like API (MarshalBinary([]byte) ([]byte, error)) then also a lot of high performance and internal parsing operations could be BinaryMarshaler implementations, allowing more standard interface reuse.

The usability is not much worse, as nil can be passed to get the exact same behavior, and there's precedent in hash.Hash.Sum.

@FiloSottile FiloSottile added the Go2 label Apr 1, 2018

@kevinburke

This comment has been minimized.

Copy link
Contributor

kevinburke commented Apr 1, 2018

I ran into the same problem when implementing github.com/kevinburke/nacl and for the most part I chose the user-friendly API over the one with the best performance. In fact I believe you commented somewhere when I debuted that library pointing out that exact problem.

Is this something that could be better implemented within the compiler? Or with some pattern that's invisible to the user?

@dsnet

This comment has been minimized.

Copy link
Member

dsnet commented Apr 2, 2018

Also MarshalJSON and MarshalText for consistency (assuming they continue to exist).

@odeke-em odeke-em added the Proposal label Apr 2, 2018

@magical

This comment has been minimized.

Copy link
Contributor

magical commented Apr 2, 2018

Regarding Hash.Sum, see also #21070, which proposes either renaming the Sum method to AppendSum or else changing it to just return a new slice and not append.

@dsnet dsnet changed the title encoding: BinaryMarshaler should be an append API proposal: encoding: BinaryMarshaler should be an append API Apr 2, 2018

@gopherbot gopherbot added this to the Proposal milestone Apr 2, 2018

@bcmills

This comment has been minimized.

Copy link
Member

bcmills commented Apr 2, 2018

It would probably be easier to migrate if the Append method has a different name (such as AppendBinary / AppendText / AppendJSON).

This is one case where default methods (#23185) are somewhat appealing, although of course the packages that check for Marshal methods could always check for both interfaces.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

ianlancetaylor commented May 1, 2018

This seems like a good thing to do. We have to decide whether to change MarshalBinary and friends, or simply add new methods.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment