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

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

Merged
merged 1 commit into from
Aug 29, 2020
Merged

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

merged 1 commit into from
Aug 29, 2020

Conversation

achanda
Copy link
Owner

@achanda achanda commented Aug 27, 2020

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

Closes #111

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

Closes #111
@achanda achanda merged commit c26c836 into master Aug 29, 2020
@achanda achanda deleted the const-fn branch August 29, 2020 10:19
@faern
Copy link
Contributor

faern commented Aug 30, 2020

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
Copy link
Owner Author

achanda commented Aug 31, 2020

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

@faern
Copy link
Contributor

faern commented Sep 1, 2020

I see now that Option::unwrap is a const fn on nightly behind a feature gate (#![feature(const_option)]): rust-lang/rust#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
Copy link
Contributor

faern commented Sep 1, 2020

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

@faern
Copy link
Contributor

faern commented Sep 1, 2020

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
Copy link
Contributor

faern commented Sep 2, 2020

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 😅

@jethrogb
Copy link
Contributor

jethrogb commented Apr 18, 2024

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

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.

const fn constructors
3 participants