-
Notifications
You must be signed in to change notification settings - Fork 35
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
Use netip.Addr instead of net.IP #36
Conversation
Before the change:
After the change:
Some observations:
|
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.
Thank you! Left a few nits. Would like to get this merged so we can make a long-overdue release this week. 🙂
Note that I also made some changes today, please give this a rebase. (I fixed the (Warning: unused-parameter: parameter 't' seems to be unused, consider removing or renaming it as _ (revive)
linter error)
@ti-mo thanks for the quick review. I will address your comments today. Note that I think it may be a good idea to have one minor release (v0.5.0) before merging this change, and one minor release (v0.6.0) just after merging this change. v0.5.0 would be the last release to support |
Yep, was considering that as well. I'll bump the Go version and do a few more dependency updates, then release 0.5.0. |
31ad178
to
215a9e0
Compare
@ti-mo I have addressed your comments, save for the one about |
Pull Request Test Coverage Report for Build 6534364355
💛 - Coveralls |
The benchmark was not correct as all iterations except for the first one were no-ops. The netlink.AttributeDecoder cannot be reused multiple times. The existing code was claiming to make a copy of it for each iteration, but mustDecodeAttributes actually returns a pointer, so it needs to be dereferenced to be copied. Signed-off-by: Antonin Bas <[email protected]>
Signed-off-by: Antonin Bas <[email protected]>
Using the net/netip package instead of the net package can help reduce the memory footprint of the library and help reduce the number of heap allocations. This is a breaking change for consumers of the library as exported types are updated to use fields of type netip.Addr instead of net.IP. We also remove the depedency on go-cmp and use assert.Equal instead in tests. Fixes ti-mo#35 Signed-off-by: Antonin Bas <[email protected]>
I gave this a rebase and a |
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 for taking this on. Looks great!
Using the net/netip package instead of the net package can help reduce
the memory footprint of the library and help reduce the number of
heap allocations.
This is a breaking change for consumers for the libray as exported types
are updated to use fields of type netip.Addr instead of net.IP.
We also include additional benchmarks to better understand the impact
of this change.
Fixes #35