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

proposal: encoding/binary: 32-bit and 16-bit values #53353

Closed
alzeih opened this issue Jun 13, 2022 · 13 comments
Closed

proposal: encoding/binary: 32-bit and 16-bit values #53353

alzeih opened this issue Jun 13, 2022 · 13 comments

Comments

@alzeih
Copy link

alzeih commented Jun 13, 2022

encoding/binary has constants defined for varints of sizes 16, 32, and 64, but no corresponding functions for 16 and 32.

const (
MaxVarintLen16 = 3
MaxVarintLen32 = 5
MaxVarintLen64 = 10
)

I propose the following new functions to be implemented for src/encoding/binary/varint.go

Uvarint32

func Uvarint32(buf []byte) (uint32, int) {}
func ReadUvarint32(r io.ByteReader) (uint32, error) {}

Varint32

func Varint32(buf []byte) (int32, int) {}
func ReadVarint32(r io.ByteReader) (int32, error) {}

Uvarint16

func Uvarint16(buf []byte) (uint16, int) {}
func ReadUvarint16(r io.ByteReader) (uint16, error) {}

Varint16

func Varint16(buf []byte) (int16, int) {}
func ReadVarint16(r io.ByteReader) (int16, error) {}
@gopherbot gopherbot added this to the Proposal milestone Jun 13, 2022
@randall77
Copy link
Contributor

AppendVarint32(buf, x) is identical to AppendVarint(buf, int64(x)) (where x is an int32). Similar for the other functions, I believe. I'm not sure we need separate functions.

@alzeih
Copy link
Author

alzeih commented Jun 13, 2022

ReadUvarint provides bounds checking based on the MaxVarintLen64. Does the alternative offered provide this checking ability?

@alzeih
Copy link
Author

alzeih commented Jun 13, 2022

@randall77 Thanks for the feedback. I've removed the AppendUvarint and PutUvarint functions from the proposal as the bounds checking only applies to the Uvarint and ReadUvarint functions.

@randall77
Copy link
Contributor

@alzeih That's a fair point for the reading variants.

@ianlancetaylor ianlancetaylor added this to Incoming in Proposals (old) Jun 13, 2022
@rsc
Copy link
Contributor

rsc commented Jun 15, 2022

We've gotten through over ten years without the lack of these being a problem. If there's a performance issue with AppendVarint(buf, int64(x)) for int32-valued x, maybe we can look at the implementation. But it doesn't seem like we need APIs for smaller ints, which are becoming less and less common.

@rsc
Copy link
Contributor

rsc commented Jun 15, 2022

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@rsc rsc moved this from Incoming to Active in Proposals (old) Jun 15, 2022
@alzeih
Copy link
Author

alzeih commented Jun 15, 2022

As previously discussed, this proposal is concerned with reading varints, not writing them.

What about a func that allows for a caller specified maximum number of bytes to read? This would work for legacy (16, 32-bit), non-standard (24-bit?), and future (128-bit) varint sizes. The existing constants could be provided as an argument.

@ianlancetaylor
Copy link
Contributor

What is the actual use case here?

Can you just read a varint and then verify that it is in range?

@alzeih
Copy link
Author

alzeih commented Jun 15, 2022

Reading a uvarint32 from a binary file format and not wanting the func to take more than it should out of the buffer, which would affect reading the rest of the file.

@ianlancetaylor
Copy link
Contributor

The varint encoding is self-delimiting. The limit of MaxVarintLen64 is to catch erroneous data. For correct data it will never be used. For erroneous data, it doesn't matter whether it affects reading the rest of the file, because the data is erroneous anyhow.

@alzeih
Copy link
Author

alzeih commented Jun 16, 2022

Okay. I was under the impression that reading more of the buffer than intended could be a bad thing (#41185). I can see this proposal is unlikely to proceed based on the feedback. Thanks for the input.

@rsc
Copy link
Contributor

rsc commented Jun 22, 2022

Based on the discussion above, this proposal seems like a likely decline.
— rsc for the proposal review group

@rsc rsc moved this from Active to Likely Decline in Proposals (old) Jun 22, 2022
@rsc rsc moved this from Likely Decline to Declined in Proposals (old) Jul 1, 2022
@rsc
Copy link
Contributor

rsc commented Jul 1, 2022

No change in consensus, so declined.
— rsc for the proposal review group

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Declined
Development

No branches or pull requests

5 participants