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
| return | ||
| } | ||
| rv.SetInt(num) | ||
| rv.SetInt(int64(unum)) |
There was a problem hiding this comment.
should we also check that unum <= math.MaxInt64 ?
There was a problem hiding this comment.
Thanks. Yes, you are right, we should!
There was a problem hiding this comment.
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.
| rv.SetInt(num) | ||
| } else { | ||
| num, _n, err = DecodeVarint(bz) | ||
| var unum uint64 |
There was a problem hiding this comment.
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.
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 |
| // cdc.decodeReflectBinary | ||
|
|
||
| var ( | ||
| ErrOverflowInt = errors.New("encoded integer value value overflows int(32)") |
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).