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

feat: add custom type support for int and uint #4

Merged
merged 1 commit into from
Aug 11, 2021

Conversation

yihau
Copy link
Contributor

@yihau yihau commented Aug 5, 2021

I found that It will panic when I serialize and deserialize including custom type. I add some support for these types:

  • uint8
  • uint16
  • uint32
  • uint64
  • int8
  • int16
  • int32
  • int64

borsh.go Outdated Show resolved Hide resolved
@yihau yihau force-pushed the feat-add-custom-type-support branch from 555a0a5 to 1672340 Compare August 5, 2021 09:04
Copy link
Collaborator

@frol frol left a comment

Choose a reason for hiding this comment

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

@yihau Thanks for the contribution!

I don't have deep experience with Go to reason about all the shortcomings, but the code looks reasonable to me 👍

borsh.go Show resolved Hide resolved
@ouromoros
Copy link
Contributor

It looks correct to me now. Great!

Last thing, I wonder if first calling v.Int() and then converting to the according type would have any performance impact. Perhaps should run a benchmark on it and see the difference.

@yihau
Copy link
Contributor Author

yihau commented Aug 6, 2021

here is the benchstat between master and my branch

name               old time/op  new time/op  delta
Deserialize-8      1.41µs ± 0%  1.69µs ± 0%   ~     (p=1.000 n=1+1)
Serialize-8         648ns ± 0%   623ns ± 0%   ~     (p=1.000 n=1+1)
FuzzSerialize-8    50.2µs ± 0%  46.6µs ± 0%   ~     (p=1.000 n=1+1)
FuzzDeserialize-8  4.65µs ± 0%  4.66µs ± 0%   ~     (p=1.000 n=1+1)

serialize is more efficient because get the value directly
there is a little bit performance impact to the deserialize function because of converting

@ouromoros
Copy link
Contributor

LGTM

@yihau
Copy link
Contributor Author

yihau commented Aug 11, 2021

Hi, is there any news on this?

@ouromoros
Copy link
Contributor

I think it's safe to merge now. Thanks for the contribution! @yihau

@ouromoros ouromoros merged commit 9705ed1 into near:master Aug 11, 2021
@yihau yihau deleted the feat-add-custom-type-support branch August 11, 2021 03:40
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.

3 participants