Skip to content

Create 0 size byte slices on allownil []byte elements#363

Merged
klauspost merged 3 commits intotinylib:masterfrom
klauspost:make-zero-byte-slices
Sep 25, 2024
Merged

Create 0 size byte slices on allownil []byte elements#363
klauspost merged 3 commits intotinylib:masterfrom
klauspost:make-zero-byte-slices

Conversation

@klauspost
Copy link
Copy Markdown
Collaborator

@klauspost klauspost commented Sep 18, 2024 •
edited
Loading

When allownil is specified on []byte elements, create 0 byte slices when a 0 sized binary blob is read.

This makes []byte fully symmetric for roundtrips.

(Tests regression and also that fields are nilled correctly)

Fixes #362

Decoder:

		case "abyteslice":
			if msgp.IsNil(bts) {
				bts = bts[1:]
				z.AByteSlice = nil
			} else {
				z.AByteSlice, bts, err = msgp.ReadBytesBytes(bts, z.AByteSlice)
				if err != nil {
					err = msgp.WrapError(err, "AByteSlice")
					return
				}
				if z.AByteSlice == nil {
					z.AByteSlice = make([]byte, 0)
				}
			}

Unmarshal:

		case "abyteslice":
			if msgp.IsNil(bts) {
				bts = bts[1:]
				z.AByteSlice = nil
			} else {
				z.AByteSlice, bts, err = msgp.ReadBytesBytes(bts, z.AByteSlice)
				if err != nil {
					err = msgp.WrapError(err, "AByteSlice")
					return
				}
				if z.AByteSlice == nil {
					z.AByteSlice = make([]byte, 0)
				}
			}

@klauspost
Create 0 size byte slices
dbe715a 
As with other types, create a 0 size byte slice when the serialized data says so.

This is a functional change, and will require the use of `allownil` to retain the existing behaviour (ie no allocs on nil/zero length slices).

Fixes tinylib#362
@klauspost klauspost requested a review from philhofer September 18, 2024 08:29
@philhofer
Copy link
Copy Markdown
Member

philhofer commented Sep 24, 2024

Adopting this would create some odd asymmetry with the default behavior on the encoder side of things which always encodes slices as zero-length arrays, even if they are nil.

IMO if we are going to introduce a behavioral change, we should do a major version bump and clean up all of that stuff at once.

@klauspost
Copy link
Copy Markdown
Collaborator Author

klauspost commented Sep 24, 2024

@philhofer Yeah, I see your point. Maybe we should put it on the allownil generation, so it will apply only to those? That would AFAICT make it fully symmetric.

Let me see if that is feasible.

@klauspost klauspost changed the title Create 0 size byte slices Create 0 size byte slices on allownil []byte elements Sep 24, 2024
@klauspost
Copy link
Copy Markdown
Collaborator Author

klauspost commented Sep 24, 2024

@philhofer Description updated. General change reverted and only allownil functionality has changed. PTAL when possible.

@klauspost klauspost merged commit 3dc88ae into tinylib:master Sep 25, 2024
@klauspost klauspost deleted the make-zero-byte-slices branch September 25, 2024 07:33
klauspost added a commit to klauspost/msgp that referenced this pull request Oct 22, 2024
@klauspost
Fix allownil propagated to byte slices as slice elements
483195a 
`[][]byte` would assume that the byte slices had allownil set. This would cause an extra `if x == nil {x = make([]byte,0)}` to be inserted on elements.

Regression from tinylib#363
@klauspost klauspost mentioned this pull request Oct 22, 2024
Merged
klauspost added a commit that referenced this pull request Oct 22, 2024
@klauspost
Fix allownil propagated to byte slices as slice elements (#374)
65798b6 
`[][]byte` would assume that the byte slices had allownil set. This would cause an extra `if x == nil {x = make([]byte,0)}` to be inserted on elements.

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

Reviewers

@philhofer philhofer philhofer approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Allownil in []byte is useless

2 participants

@klauspost @philhofer