search

achanda / ipnetwork

A library to work with CIDRs in rust
Apache License 2.0
121 stars 38 forks source link

Set difference of two `IpNetwork`s #125

Open jvff opened 4 years ago

jvff commented 4 years ago

We had a need to calculate the set difference between two IpNetworks. So given an initial broad IpNetwork, we wanted to remove a smaller IpNetwork sub-network from it. The operation returned an iterator over the resulting set of IpNetworks.

We have code for this implemented, but we were wondering if this feature is something that would be worthwhile upstreaming. Should I open a PR with the code?

achanda commented 4 years ago

Yes, this is definitely something that is useful for the broader community. PR is welcome, thanks!

jvff commented 4 years ago

Great, thanks! I haven't opened a PR yet because I'm not sure how to proceed. The initial implementation I had was very close to this one, except it didn't actually implement Sub for the IP network types, only the custom IpNetworkSub trait. I can also remove the Sub implementation and just use the custom trait, not sure if having the - operator working with these semantics makes sense.

The idea for this implementation was to reduce the repeated code at the cost of more abstractions and boilerplate code. The goal was to keep the actual operation in one location, so that it's easier to review and to make changes in the future. However, it has the major downside of actually exporting those new abstractions :/

Another solution would be to simplify it by removing the abstractions and simply copying the code. It's duplicated, but it does become simpler.

Let me know which solution you'd prefer, and I can open a PR to start discussing any changes that should be made :)

jvff commented 4 years ago

Sorry for sending out a large code dump :sweat_smile: Would it be okay if I opened a PR using the second solution, which should be simpler?

achanda commented 4 years ago

Sorry for being late @jvff and thanks for your work on this. I feel like the second option is more readable and maintainable than the first one. Let me know if you agree. Please feel free to open a PR if you have time to get to it.

jvff commented 4 years ago

No worries! Thanks for your time on this :)

I've opened a PR (#126) with the implementation. Should this issue be closed and the discussion and review continue there?

CameronNemo commented 4 years ago

You may be interested in "Radix" trees.

https://github.com/deepfield/py-radix/blob/master/radix.c

https://github.com/ibmandura/ArtTree

I am looking to use them with IP prefixes, and the status quo is an old C library from 20 years ago. (seriously there are go, perl, ruby, python bindings for this little BSD-4-Clause licensed library).