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

Fix varint marshal, unmarshall #309

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

illia-li
Copy link

@illia-li illia-li commented Oct 15, 2024

Fixed for varint type:

  1. string marshal and unmarshal as nullable.
    1.1. Before: marshals "" - caused an error; unmarshals nil data into "0".
    1.2. Now: marshals and unmarshals nil data - "".
  2. Big string (bigger then math.MaxInt64) marshal and unmarshal, before not supported, now supported.
  3. custom string unmarshals, before not supported, now supported.
  4. For all go types marshals and unmarshal functions are optimized.
  5. Marshal data, which equal math.MaxUint64, into uint64, uint, return error before, now no error.

@illia-li illia-li force-pushed the il/fix/marshal/varint branch 2 times, most recently from 3ee15a2 to 6b215f1 Compare October 15, 2024 16:29
@dkropachev dkropachev self-requested a review October 15, 2024 17:44
serialization/varint/marshal_custom.go Outdated Show resolved Hide resolved
marshal.go Outdated Show resolved Hide resolved
Comment on lines +115 to +134
switch {
case byte(v>>63) != 0:
return []byte{0, byte(v >> 56), byte(v >> 48), byte(v >> 40), byte(v >> 32), byte(v >> 24), byte(v >> 16), byte(v >> 8), byte(v)}
case byte(v>>55) != 0:
return []byte{byte(v >> 56), byte(v >> 48), byte(v >> 40), byte(v >> 32), byte(v >> 24), byte(v >> 16), byte(v >> 8), byte(v)}
case byte(v>>47) != 0:
return []byte{byte(v >> 48), byte(v >> 40), byte(v >> 32), byte(v >> 24), byte(v >> 16), byte(v >> 8), byte(v)}
case byte(v>>39) != 0:
return []byte{byte(v >> 40), byte(v >> 32), byte(v >> 24), byte(v >> 16), byte(v >> 8), byte(v)}
case byte(v>>31) != 0:
return []byte{byte(v >> 32), byte(v >> 24), byte(v >> 16), byte(v >> 8), byte(v)}
case byte(v>>23) != 0:
return []byte{byte(v >> 24), byte(v >> 16), byte(v >> 8), byte(v)}
case byte(v>>15) != 0:
return []byte{byte(v >> 16), byte(v >> 8), byte(v)}
case byte(v>>7) != 0:
return []byte{byte(v >> 8), byte(v)}
default:
return []byte{byte(v)}
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why encUint64 and encUint are the same ?

Copy link
Author

@illia-li illia-li Oct 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

uint max bit size same as uint64 for 64-bit architectures

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

exactly, why not to use same function ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are inputs are different uint and uint64.
I don`t want to use unnecessary type convert.

@illia-li illia-li force-pushed the il/fix/marshal/varint branch 3 times, most recently from 12557fa to b063b8e Compare October 17, 2024 03:58
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 this pull request may close these issues.

2 participants