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.

Sign up for GitHub

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

encoding/binary: document that ByteOrder must be non-nil #64386

Closed
3052 opened this issue Nov 26, 2023 · 6 comments
Closed

encoding/binary: document that ByteOrder must be non-nil #64386

3052 opened this issue Nov 26, 2023 · 6 comments
Labels
Documentation NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.

Comments

@3052
Copy link

3052 commented Nov 26, 2023

Proposal Details

this panics as expected:

package main

import (
   "bytes"
   "encoding/binary"
)

func main() {
   r := bytes.NewReader([]byte{1,2})
   var i int16
   binary.Read(r, nil, &i)
}

but this does not:

var i int8
binary.Read(r, nil, &i)

I think the docs should reflect that ByteOrder can be nil if the integer width is one byte.

https://godocs.io/encoding/binary#Read
@3052 3052 added the Proposal label Nov 26, 2023
@mateusz834 mateusz834 added Documentation NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. and removed Proposal labels Nov 26, 2023
@mateusz834
Copy link
Member

This does not need to go through the proposal process, feel free to send a CL.

@AlexanderYastrebov
Copy link
Contributor

I think the docs should reflect that ByteOrder can be nil if the integer width is one byte.

I'd say it should panic on nil ByteOrder for consistency

@bcmills
Copy link
Contributor

bcmills commented Nov 27, 2023

I think the docs should reflect that ByteOrder can be nil if the integer width is one byte.

The current documentation says: “Bytes read from r are decoded using the specified byte order and written to successive fields of the data.” To me that does not allow for the ByteOrder to be nil: it says “are decoded” (not “may be decoded”) and “the specified byte order” (not “the specified byte order, if any”).

I'd say it should panic on nil ByteOrder for consistency

In my experience, it is pretty much always a mistake for documentation to promise to panic if an invalid argument is used, because that prevents future changes that give defined behavior to previously-invalid arguments.

If anything, perhaps we should clarify as: “… decoded using the specified byte order, which must be non-nil, and written …”.

@3052
Copy link
Author

3052 commented Nov 27, 2023

I agree with @bcmills - I would prefer nil to be explicitly allowed in some cases, but failing that, documenting that nil must not be used is a good option as well

@merykitty
Copy link

Under-enforcing is also not good since there will be people depending on the undefined behaviour which creates friction for any kind of change in both directions.

@AlexanderYastrebov
Copy link
Contributor

Adding nil check for int8 case impacts performance by -10%

@adonovan adonovan changed the title encoding/binary: clarify when ByteOrder can be nil encoding/binary: document that ByteOrder must be non-nil Dec 22, 2023
@3052 3052 closed this as completed Jan 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Documentation NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@AlexanderYastrebov @bcmills @mateusz834 @merykitty @3052