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

Allownil in []byte is useless #362

Closed
lxyshuai opened this issue Sep 18, 2024 · 6 comments · Fixed by #363
Closed

Allownil in []byte is useless #362

lxyshuai opened this issue Sep 18, 2024 · 6 comments · Fixed by #363

Comments

@lxyshuai
Copy link

No description provided.

@lxyshuai
Copy link
Author

msgp

package main

import (
	"fmt"
)

//go:generate msgp

type A struct {
	Bytes   []byte   `msg:"bytes,omitempty,allownil"`
	Strings []string `msg:"strings,omitempty,allownil"`
	Ints    []int    `msg:"ints,omitempty,allownil"`
}

func main() {
	a := A{
		Bytes:   make([]byte, 0),
		Strings: make([]string, 0),
		Ints:    make([]int, 0),
	}

	aBytes, _ := a.MarshalMsg(nil)
	aTemp := A{}
	aTemp.UnmarshalMsg(aBytes)

	fmt.Println(aTemp)
}

Allownil in []byte is useless

@klauspost
Copy link
Collaborator

klauspost commented Sep 18, 2024

omitempty and allownil doesn't make sense together, since both operate on the nil value. Adding it will do:

	// check for omitted fields
	zb0001Len := uint32(3)
	var zb0001Mask uint8 /* 3 bits */
	_ = zb0001Mask
	if z.Bytes == nil {
		zb0001Len--
		zb0001Mask |= 0x1
	}

Resulting in the same output, since all are included as 0 length arrays, and therefore it will never emit nil values.

omitempty will not omit 0-length slices. This is stated in the docs is that it checks against the zero value. The zero value of a []byte is nil.

@klauspost
Copy link
Collaborator

There is a quirk in ReadBytesBytes, where if the array size is 0 a new slice is not allocated if scratch is nil. I will send a fix for that.

@lxyshuai
Copy link
Author

lxyshuai commented Sep 18, 2024

@klauspost Thank you!
image
when scratch is nil,readBytesBytes return nil

@klauspost
Copy link
Collaborator

Yes.

@lxyshuai
Copy link
Author

Should we reopen this issue?

@klauspost klauspost reopened this Sep 18, 2024
klauspost added a commit to klauspost/msgp that referenced this issue Sep 18, 2024
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants