-
Notifications
You must be signed in to change notification settings - Fork 78
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
Make default int encoding varint (not zigzag) #238
Conversation
- minimal changes to ensure proto3 compat. for unsigned ints (int, int32, int64) - fuzzer fails in many places
binary-decode.go
Outdated
if slide(&bz, &n, _n) && err != nil { | ||
return | ||
} | ||
rv.SetInt(num) | ||
rv.SetInt(int64(unum)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we also check that unum <= math.MaxInt64
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Yes, you are right, we should!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh actually, this wouldn't make any sense in this case. Any negative value num will certainly be > math.MaxInt64 (all negative nums get encoded "after" MaxInt64). But checks make sense in the cases of int32
(and depending on the platform, also for int
). I'll add those.
binary-decode.go
Outdated
@@ -115,11 +115,12 @@ func (cdc *Codec) decodeReflectBinary(bz []byte, info *TypeInfo, rv reflect.Valu | |||
} | |||
rv.SetInt(num) | |||
} else { | |||
num, _n, err = DecodeVarint(bz) | |||
var unum uint64 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should not be called a unum. Unums are a cool proposal to replace floats. https://en.wikipedia.org/wiki/Unum_(number_format).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Universal numbers ... Thanks! Learned something new. I'll rename to u64
or sth like this.
Addressed all comments and updated the changelog for another release. Want to have another look? Or good to merge @melekes @ValarDragon |
binary-decode.go
Outdated
@@ -12,6 +13,16 @@ import ( | |||
//---------------------------------------- | |||
// cdc.decodeReflectBinary | |||
|
|||
var ( | |||
ErrOverflowInt = errors.New("encoded integer value value overflows int(32)") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
value value
First stab on #237
related to tendermint/tendermint#2682
I propose to do a followup PR to add back the zigzag encoding option (via a tag).