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

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

Closed
FiloSottile opened this issue Apr 1, 2018 · 7 comments
Closed

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

FiloSottile opened this issue Apr 1, 2018 · 7 comments
Labels
NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. Proposal v2 A language change or incompatible library change
Milestone
Proposal

Comments

@FiloSottile
Copy link
Contributor

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 v2 A language change or incompatible library change label Apr 1, 2018
@kevinburke
Copy link
Contributor

kevinburke commented Apr 1, 2018 •
edited

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
Copy link
Member

dsnet commented Apr 2, 2018

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

@magical
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
Copy link
Contributor

bcmills commented Apr 2, 2018 •
edited

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 ianlancetaylor added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label May 1, 2018
@ianlancetaylor
Copy link
Contributor

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

@gopherbot gopherbot removed the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Aug 16, 2019
@gopherbot gopherbot added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Sep 3, 2019
@ianlancetaylor
Copy link
Contributor

At this point we can't realistically change the encoding.BinaryMarshaler interface. We could introduce a new interface with an append API. Anybody want to make that proposal? In the meantime I'm going to close this one. Thanks.

@ianlancetaylor ianlancetaylor closed this as not planned Won't fix, can't repro, duplicate, stale Aug 23, 2023
@dsnet
Copy link
Member

dsnet commented Aug 30, 2023

I filed #62384.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. Proposal v2 A language change or incompatible library change
Projects
None yet
Milestone
Proposal
Development

No branches or pull requests
8 participants
@magical @kevinburke @FiloSottile @ianlancetaylor @odeke-em @bcmills @dsnet @gopherbot