achanda / ipnetwork

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

Convert Ipv{4,6}Network::new to const functions #137

Closed achanda closed 4 years ago

achanda commented 4 years ago

IpNetwork::new cannot be const since it is not supported with the ? operator

Closes #111

faern commented 4 years ago

Too bad Result::unwrap is not a const fn. I don't know if it ever will be, but last time I looked at it it looked pretty far away from being possible. This means that even after this PR it's not possible to store an Ipv4Network in a constant sadly. I'm not sure what the best way to solve that would be.

Maybe it's not worth working around it even. Not sure how important it would be to be able to have IP networks in constants, it still works to have it as a lazy_static. But if one would try to work around it, I wonder what the best approach would be. const fn new_saturating(...) -> Self would be one way. Where the prefix is just clamped to the maximum prefix if it's outside the allowed range.

achanda commented 4 years ago

@faern I will keep a watch on replies here to see if that is something people might need.

faern commented 4 years ago

I see now that Option::unwrap is a const fn on nightly behind a feature gate (#![feature(const_option)]): https://github.com/rust-lang/rust/issues/67441.

So it would be possible to have Ipv4Network::new_somethingsomething(..) -> Option<Self>. The only difference from the current new is that as a user you don't get an error type back that implements std::error::Error with an error message we decide on in this library. Instead users would kind of need to tuck on some ok_or(TheirError::InvalidNetwork) or whatever they want to do to make it into a Result.

I'm not proposing we add this special constructor now. I just want to drop the idea and link here, so const_option can be monitored. I think we need to think about the design here a bit, and also const_option should mature and probably be stabilized before this makes a ton of sense.

faern commented 4 years ago

The standard library NonZeroXX::new constructors return Options. So going with that design would not be unheard of :shrug: https://doc.rust-lang.org/stable/std/num/struct.NonZeroU8.html#method.new

faern commented 4 years ago

Aaah. Wait.. If Result::ok can be made into a const fn we don't need a special constructor. It would be possible to do Ipv4Network::new(...).ok().unwrap() to go via an Option and unwrap it. That's probably cleaner than having a different constructor.

faern commented 4 years ago

Doh.... Another roadblock. Making Result::ok a const fn is not as trivial as I would have hoped. Methods taking ownership of self can't be const fns yet. Since they can't run drop implementations. I guess the constification of Rust still has some way to go :sweat_smile:

jethrogb commented 5 months ago

Too bad Result::unwrap is not a const fn.

That plus the chosen error type makes this function pretty useless? Is there any way to use Ipv{4,6}Network::new in a const context?

Even if you use match, you get this error:

error[E0493]: destructor of `Result<Ipv4Network, IpNetworkError>` cannot be evaluated at compile-time