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

Allow CodedOutputStream to use Array Slices #7839

Closed
wants to merge 2 commits into from

Conversation

CauhxMilloy
Copy link
Contributor

The constructor for CodedOutputStream (which takes in a given array offset) is private. As it stands, it is only used via one other constructor (which always passes 0 as the offset), making the offset always hard-coded to the initial value of 0. Making this constructor public allows for client applications to serialize messages into arrays without superfluous array copies. It is also worth noting that the corresponding constructor on CodedInputStream is already public (public CodedInputStream(byte[] buffer, int offset, int length)).

Looking through the history, it seems that this constructor was originally marked private when the library used static factory methods. When migrating over to using constructors directly, this constructor was not made public (perhaps because there was no corresponding factory method?).

The constructor for `CodedOutputStream` (which takes in a given array offset) is `private`. As it stands, it is only used via one other constructor (which always passes 0 as the offset), making the offset always hard-coded to the initial value of 0. Making this constructor `public` allows for client applications to serialize messages into arrays without superfluous array copies. It is also worth noting that the corresponding constructor on `CodedInputStream` is already public (`public CodedInputStream(byte[] buffer, int offset, int length)`).

Looking through the history, it seems that this constructor was originally marked private when the library used static factory methods. When migrating over to using constructors directly, this constructor [was not made public](protocolbuffers@0e0e0c9#diff-edc376d7570e8db3f8d8745ef37719f3R82) (perhaps because there was no corresponding factory method?).
@haberman
Copy link
Member

We should understand why this wasn't made public to begin with, or if there is a preferred alternative interface that is public.

@CauhxMilloy
Copy link
Contributor Author

CauhxMilloy commented Jan 1, 2021

We should understand why this wasn't made public to begin with, or if there is a preferred alternative interface that is public.

CodedInputStream has the same constructor available. It has some extra parameter checks, I'll add those to this PR. Otherwise, there doesn't seem to be an issue with making this public, and it might be preferred over an alternative in order to stay consistent with CodedInputStream.

Throwing `ArgumentOutOfRangeException` when bad parameters are given. This is consistent with checks made in corresponding constructor in [CodedInputStream](https://github.com/protocolbuffers/protobuf/blob/master/csharp/src/Google.Protobuf/CodedInputStream.cs#L101).
@CauhxMilloy
Copy link
Contributor Author

Hi, this change is still desired. This PR has been here for almost a year with no reply. What next steps can be taken?

Thanks!

@fowles fowles assigned jtattermusch and unassigned perezd Aug 22, 2021
@jtattermusch
Copy link
Contributor

I'm not sure if this is still needed as CodedOuputStream is now considered legacy (since the introduction of #7576) - better and faster APIs are now available.

You can now write messages directly to a IBufferWriter or a Span, which is faster and saves any unnecessary allocations. Use of those APIs is preferred over using CodedOutputStream.

public static void WriteTo(this IMessage message, IBufferWriter<byte> output)

public static void WriteTo(this IMessage message, Span<byte> output)

@acozzette
Copy link
Member

I'm going to close this since according to @jtattermusch's comment there are now better APIs available.

@acozzette acozzette closed this Oct 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants